Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted

[Laszlo Ersek <lersek@redhat.com>]

On 06/08/17 12:11, Ard Biesheuvel wrote:
> On 7 June 2017 at 23:10, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/06/17 20:16, Laszlo Ersek wrote:
>>> On 06/05/17 19:47, Jordan Justen wrote:
>>>> On 2017-06-03 08:42:03, Laszlo Ersek wrote:
>>>>> ... by narrower than 8-byte ADD_POINTER references.
>>>>>
>>>>> Introduce the CollectRestrictedAllocations() function, which iterates over
>>>>
>>>> How about Collect32BitRestrictedAllocations and similar treatment for
>>>> other names that just say 'restricted'?
>>>
>>> Something like this crossed my mind, but I didn't know how to prefix the
>>> simple variable / parameter names "RestrictedAllocations" with "32Bit",
>>> as the identifiers cannot start with a digit.
>>>
>>> I even thought of spelling it out, as in
>>> "ThirtyTwoBitRestrictedAllocations", but that seemed ridiculous.
>>>
>>> Prefixing "32Bit" with an underscore, _32Bit, looks ugly, plus the C
>>> standard actually reserves it:
>>>
>>>     All identifiers that begin with an underscore are always reserved
>>>     for use as identifiers with file scope in both the ordinary and tag
>>>     name spaces.
>>>
>>> While I'd only use this variable name as function parameter / local
>>> variable, and thereby I'd shadow any such impl. defined global variable
>>> ("identifiers with file scope"), the shadowing would trigger a compiler
>>> warning for sure, and break the build.
>>>
>>> What do you suggest?
>>
>> Ultimately I went with
>>
>>   s/RestrictedAllocations/AllocationsRestrictedTo32Bit/
>>
>> in the patch body and in the commit message too. Cleaned up the line
>> lengths and such as well, plus retested the patch.
>>
>> Commit 4275f38507a4.
>>
> 
> Hi Laszlo,
> 
> Thanks again for the effort. Sadly, though, this patch is breaking my CI build:
> 
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c: In function 'InstallQemuFwCfgTables':
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:357:29: error:
> 'AllocationsRestrictedTo32Bit' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>    if (OrderedCollectionFind (
>                              ^
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:975:23: note:
> 'AllocationsRestrictedTo32Bit' was declared here
>    ORDERED_COLLECTION       *AllocationsRestrictedTo32Bit;
>                        ^
> 

Indeed, Gerd's Jenkins CI (using GCC49) reported the same (non-)issue.

It is a compiler bug. The compiler reports that the ProcessCmdAllocate()
function may be called with "AllocationsRestrictedTo32Bit"
uninitialized. That's not the case; this function call is only reached
if CollectAllocationsRestrictedTo32Bit() returns EFI_SUCCESS --
otherwise we jump to the "FreeLoader" label, way past the
ProcessCmdAllocate() call --, and when
CollectAllocationsRestrictedTo32Bit() succeeds, it will have
"AllocationsRestrictedTo32Bit" assigned.

In order to expedite things, could you please help me by submitting a
one-liner patch? Namely, please set "AllocationsRestrictedTo32Bit" to
NULL right before the CollectAllocationsRestrictedTo32Bit() function call.

(If you add a comment that the assignment exists to suppress an invalid
compiler warning, that's even better. :) )

Thank you!
Laszlo