From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22e.google.com (mail-io0-x22e.google.com [IPv6:2607:f8b0:4001:c06::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D1F5B21A143C1 for ; Thu, 8 Jun 2017 12:04:38 -0700 (PDT) Received: by mail-io0-x22e.google.com with SMTP id t87so4531127ioe.0 for ; Thu, 08 Jun 2017 12:05:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=G9OgjGnq40B9O9AK1KDePvpTqZpRKJyUBZeCkY3pujY=; b=OboF4eAFnneAk1CLZ8jFrf0xIWKsLxAp2+U7lWG0c/4Scw8J50cw3JoQiqB8q+pXw+ HfQRn9cOJDNoVmmG1md1Qdih98wBH5p3fUaEA8qWr1iZqKpBm4/8ELYImyzcK4b0Bbno 3QNM2UX9bcy6sTgO8GOUDMBWKH+1bkrw3Mrzs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=G9OgjGnq40B9O9AK1KDePvpTqZpRKJyUBZeCkY3pujY=; b=GRj6F/4pBlfEaf5ALJ1XHdnv5jxXFfIztSFKNrko+S8c/O6/QSTD/ylblOA8ul598F fxyUBfND/rC1Kg1LbNmsxkcEw6mCshE4IM42j/Oer5bolqzCqcHbfmnzXb3ULmZhsGC/ c5izCboP7mDGCIZLwIsfSg+p5Mfyj6xRM10y64BKGzr6qtP1yLdOAfSosVMMEfN61zeC 4hR7zS7jyrqEuM9ejU9SeNhTq9NVlcvoOrvyxGbFJAp1mTIquelL1A2ejXbQU40tiuqs 2VZPKrFBvci9AiQHuq+ZQOLSpFcgP4Q35bKtmxZxPz9sx4MHRCiFe3JQFFC/fv/10ptD 9Y3Q== X-Gm-Message-State: AODbwcCVZ5VudLm/qP1wFaP41DQzxUqdP0/lXei42EtaIVTOyXzhw91Z NLoKl/ySguK6l+Gf9NWy4BY2NbKVR9hpDeE= X-Received: by 10.107.63.139 with SMTP id m133mr21503667ioa.87.1496948747319; Thu, 08 Jun 2017 12:05:47 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.164.76 with HTTP; Thu, 8 Jun 2017 12:05:46 -0700 (PDT) In-Reply-To: <29ef9616-4b75-5b37-f154-97585267c23b@redhat.com> 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> <29ef9616-4b75-5b37-f154-97585267c23b@redhat.com> From: Ard Biesheuvel Date: Thu, 8 Jun 2017 19:05:46 +0000 Message-ID: To: Laszlo Ersek Cc: Jordan Justen , edk2-devel-01 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 19:04:39 -0000 Content-Type: text/plain; charset="UTF-8" On 8 June 2017 at 18:40, Laszlo Ersek wrote: > 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. > Done Thanks, Ard.