On Fri, Apr 4, 2014 at 6:07 AM, Laszlo Ersek <ler...@redhat.com> wrote: > On 04/04/14 11:30, Reza Jelveh wrote: >> On 04/04/14 02:14, Dong, Eric wrote: >>> This error info is printed in CVfrCompiler::Compile () function. >>> And before call this function, if vfrcompiler parse the code >>> success, the status will be set to STATUS_PREPROCESSED, else it >>> will be set to STATUS_DEAD. I don't think it's necessary to initial >>> the value as STATUS_STARTED. > > Yes, it is necessary. See below. > >>> >>> Also I'm curious what's the root cause for your error and does this patch >>> fix your error? >> >> Hey, sorry I just removed all my trace notes yesterday. As far as I remember >> it never makes it to STATUS_INITIALIZED. I'm not sure what kind of bug that >> is, because i tried both gcc 4.4 and 4.8 in archlinux. So it must be some >> other environment based issue. >> >> Anyway, yes the patch fixes it. I found two other people having the same >> issue >> here: https://aur.archlinux.org/packages/ovmf-svn/ > > Reza, thank you for tracking this down; and indeed there's a bug in > VfrCompiler. > > The problem is that the CVfrCompiler::mRunStatus member, a non-static, > POD (plain old data) member, is not initialized. (COMPILER_RUN_STATUS is > an enum type.) > > Hence in the following (otherwise successful) call chain, mRunStatus has > indeterminate value, causing undefined behavior very early: > > CVfrCompiler::CVfrCompiler() > CVfrCompiler::OptionInitialization() // doesn't set mRunStatus > CVfrCompiler::IS_RUN_STATUS() > // reads mRunStatus > > Here's why CVfrCompiler::mRunStatus is not initialized, according to the > C++03 standard (I could use a C++11 draft too, but I don't think > BaseTools are built in C++11 mode): > > --------*-------- > 12 Special member functions > 12.6 Initialization > 12.6.2 Initializing bases and members > > 4 If a given nonstatic data member or base class is not named by a > mem-initializer-id (including the case where there is no > mem-initializer-list because the constructor has no > ctor-initializer), then > > -- If the entity is a nonstatic data member of (possibly > cv-qualified) class type (or array thereof) or a base class, and > the entity class is a non-POD class, the entity is default- > initialized (8.5). If the entity is a nonstatic data member of a > const-qualified type, the entity class shall have a > user-declared default constructor. > > -- Otherwise, the entity is not initialized. If the entity is of > const-qualified type or reference type, or of a (possibly > cv-qualified) POD class type (or array thereof) containing > (directly or indirectly) a member of a const-qualified type, the > program is ill-formed. > > After the call to a constructor for class X has completed, if a > member of X is neither specified in the constructor's > mem-initializers, nor default-initialized, nor value-initialized, > nor given a value during execution of the body of the constructor, > the member has indeterminate value. > --------*-------- > > In this case: > (a) CVfrCompiler::CVfrCompiler() is the constructor, > (b) mRunStatus is a nonstatic data member, > (c) the constructor has no ctor-initializer; ie. a member initializer > list, separated with a colon (:), as in > > CVfrCompiler::CVfrCompiler ( > IN INT32 Argc, > IN CHAR8 **Argv > ) : mRunStatus (STATUS_STARTED) > { > // ... > } > > (d) mRunStatus has an enumeration type, ie. a POD type; hence the > second bullet applies > (e) The ill-formedness part of the 2nd bullet doesn't apply. > > (The program is not ill-formed, but the read access to mRunStatus is > undefined.) > > I think that Reza's fix is correct, I'd just put the
Even if the fix is correct, the we can't do anything with it, since it was not submitted with Contributed-under. At the very least, Contributed-under is required, but an actual commit message would be good too. :) > SET_RUN_STATUS (STATUS_STARTED); > > statement right at the top of the constructor, instead of pushing it > down to CVfrCompiler::OptionInitialization(). > > (Another possibility would be to use a ctor-initializer, like in (c) > above, but I think that the BaseTools coding style doesn't allow that.) Is it? -Jordan ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
