I agree with Laszlo's changes based on his analysis. Thanks, Eric -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Friday, April 04, 2014 9:08 PM To: Dong, Eric; Reza Jelveh Cc: [email protected] Subject: Re: [edk2] [PATCH] vfrcompile not working on recent linux distributions
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 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.) Thanks Laszlo ------------------------------------------------------------------------------ Put Bad Developers to Shame Dominate Development with Jenkins Continuous Integration Continuously Automate Build, Test & Deployment Start a new project now. Try Jenkins in the cloud. http://p.sf.net/sfu/13600_Cloudbees _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
