From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F26D021967BE1 for ; Thu, 8 Jun 2017 11:39:49 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B2D7067ED7; Thu, 8 Jun 2017 18:40:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B2D7067ED7 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B2D7067ED7 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-122.phx2.redhat.com [10.3.116.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id B6B4D60F8F; Thu, 8 Jun 2017 18:40:57 +0000 (UTC) To: Ard Biesheuvel Cc: Jordan Justen , edk2-devel-01 References: <20170603154203.29907-1-lersek@redhat.com> <20170603154203.29907-2-lersek@redhat.com> <149668486395.8581.9154897144168590783@jljusten-skl> <5e0277a2-0bf4-e8a1-f61f-77cdb896da06@redhat.com> From: Laszlo Ersek Message-ID: <29ef9616-4b75-5b37-f154-97585267c23b@redhat.com> Date: Thu, 8 Jun 2017 20:40:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 08 Jun 2017 18:40:58 +0000 (UTC) Subject: Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jun 2017 18:39:50 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/08/17 12:11, Ard Biesheuvel wrote: > On 7 June 2017 at 23:10, Laszlo Ersek 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