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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 June 2017 at 18:40, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

Done

Thanks,
Ard.