[PATCH] Add a default constructor to work around Visual Studio compiler bug

Mon Feb 9 12:28:32 PST 2015

Hi Yaron,

Thanks again for the feedback, I'll incorporate that into the patch and then commit the change. I do not have commit access, but know others who can commit on my behalf. Thanks!

Douglas Yung

From: Yaron Keren [mailto:yaron.keren at gmail.com]
Sent: Saturday, February 07, 2015 21:28
To: Yung, Douglas
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Add a default constructor to work around Visual Studio compiler bug

Hi Douglas,

OK. I'd add a test comment  saying that the test depends on UseInitArray behaviour under FreeBSD, as coded in Generic_ELF::addClangTargetOptions() and move the Visual C++ comment in TargetLoweringObjectFileImpl.h into the test as well. No need to justify the good programming practice of member initialization in the code.

Other than this, LGTM. Do you have commit access?

Yaron

2015-02-08 4:15 GMT+02:00 Yung, Douglas <douglas_yung at playstation.sony.com<mailto:douglas_yung at playstation.sony.com>>:
>
> Hi Yaron,
>
>
>
> I think what you suggest is a good programming practice, but that is beyond the scope of this patch. This is meant to be a small fix to an issue we have actually had a problem with.
>
>
>
> The test is the best test we could come up with for this issue. At best, it will sometimes fail with the bug still present because the failure depends on whatever value happens to be at the data member's memory location when the object is created which could be 1 or 0. But it does show the failure, so it does exercise the bug.
>
>
>
> Douglas Yung
>
>
>
> From: Yaron Keren [mailto:yaron.keren at gmail.com<mailto:yaron.keren at gmail.com>]
> Sent: Tuesday, February 03, 2015 19:03
> To: Yung, Douglas
> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: [PATCH] Add a default constructor to work around Visual Studio compiler bug
>
>
>
> Hi,
>
>
>
> Regardless this Visual C++ behaviour vs. C++ rules, a program depending upon the compiler to zero data members in a default constructor looks like a bug, the programmer may have forgotten to write the default constructor and to initialize the values, (which may have been be non-zero!).
>
>
>
> The class constructor should be provided explicity and initialize data members to specific known values. So the constructor part of the patch LGTM even without the comments, no need to explain why class data members are initialized in the constructor.
>
>
>
> I don't know enough about the test part of the patch.
>
>
>
> Yaron
>
>
>
>
>
>
>
> 2015-02-04 2:06 GMT+02:00 Yung, Douglas <douglas_yung at playstation.sony.com<mailto:douglas_yung at playstation.sony.com>>:
>
> Ping. Could someone do a review of this small work-around for a Visual Studio bug?
>
>
>
> Douglas Yung
>
>
>
> From: Yung, Douglas
> Sent: Wednesday, January 28, 2015 17:46
> To: 'llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>'
> Subject: [PATCH] Add a default constructor to work around Visual Studio compiler bug
>
>
>
> Hi, I would like to submit the following patch to include/llvm/CodeGen/TargetLoweringObjectFileImpl.h which works around a bug in the Visual Studio compiler.
>
>
>
> The bug is that the compiler does not initialize a field in the class when it creates a default constructor when it should be zero-initialized. The result is that when the field is later accessed, the value is whatever value happens to be stored there, instead of the proper zero value (false in this case). The explanation of the issue is the following:
>
>
>
> Quoting The C++11 standard section [class.base.init]p8 that "In a non-delegating constructor, if a given non-static data member or base class is not designated by a mem-initializer-id (including the case where there is no mem-initializer-list because the constructor has no ctor-initializer) and the entity is not a virtual base class of an abstract class (10.4), then
>
>
>
> - if the entity is a non-static data member that has a brace-or-equal-initializer, the entity is initialized as specified in 8.5;
>
> - otherwise, if the entity is a variant member (9.5), no initialization is performed;
>
> - otherwise, the entity is default-initialized "
>
>
>
> The lack of default-initialization is what is causing this problem if we use the Visual Studio compiler:
>
> At CodeGen stage, the implicit default constructor for class TargetLoweringObjectFileELF is called. Class 'TargetLoweringObjectFileELF' has a field called 'UseInitArray'. If that field is set to true, then the compiler will emit global constructors/destructors in the .init_array section. Otherwise, it will emit those symbols in the .ctors sections.
>
>
>
> The lack of a user defined default constructor for class TargetLoweringObjectFileELF means that the compiler will have to implicitly generate a default constructor with no initializer and empty body. Then, still according to the standard ( [class.base.init]p8 ) it should default-initialize field 'UseInitArray'.
>
>
>
> The Visual Studio compiler does not perform the field initialization. Hence the intermittent problem (depending on the garbage value read from memory we either think we shall emit those symbols on the .ctors section or .init_array section).
>
>
>
> To illustrate this issue, I have attached a sample file mytest2.cpp, and the assembly generated for it by the MSVC18 (Visual Studio 2013). The source defines 3 different classes A, B and C. All are identical except for the following characteristics:
>
>
>
> *         Class A does not define a default constructor and uses one generated automatically by the compiler
>
> *         Class B defines an empty default constructor
>
> *         Class C defines an empty default constructor which initializes the Boolean field to false
>
>
>
> If you examine the assembly generated by the compiler, you can see that the constructors generated for A and B area identical, and C is similar, but it contains an initialization of the Boolean field to false which A and B are both missing.
>
>
>
> This patch works around the issue by adding a default constructor to the class which explicitly initializes the field to false (the zero value).
>
>
>
> The patch also includes an LLVM test which checks that a .init_array is not generated for a non-linux ELF target.
>
>
>
> Douglas Yung
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>

2015-02-08 4:15 GMT+02:00 Yung, Douglas <douglas_yung at playstation.sony.com<mailto:douglas_yung at playstation.sony.com>>:
>
> Hi Yaron,
>
>
>
> I think what you suggest is a good programming practice, but that is beyond the scope of this patch. This is meant to be a small fix to an issue we have actually had a problem with.
>
>
>
> The test is the best test we could come up with for this issue. At best, it will sometimes fail with the bug still present because the failure depends on whatever value happens to be at the data member's memory location when the object is created which could be 1 or 0. But it does show the failure, so it does exercise the bug.
>
>
>
> Douglas Yung
>
>
>
> From: Yaron Keren [mailto:yaron.keren at gmail.com<mailto:yaron.keren at gmail.com>]
> Sent: Tuesday, February 03, 2015 19:03
> To: Yung, Douglas
> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: [PATCH] Add a default constructor to work around Visual Studio compiler bug
>
>
>
> Hi,
>
>
>
> Regardless this Visual C++ behaviour vs. C++ rules, a program depending upon the compiler to zero data members in a default constructor looks like a bug, the programmer may have forgotten to write the default constructor and to initialize the values, (which may have been be non-zero!).
>
>
>
> The class constructor should be provided explicity and initialize data members to specific known values. So the constructor part of the patch LGTM even without the comments, no need to explain why class data members are initialized in the constructor.
>
>
>
> I don't know enough about the test part of the patch.
>
>
>
> Yaron
>
>
>
>
>
>
>
> 2015-02-04 2:06 GMT+02:00 Yung, Douglas <douglas_yung at playstation.sony.com<mailto:douglas_yung at playstation.sony.com>>:
>
> Ping. Could someone do a review of this small work-around for a Visual Studio bug?
>
>
>
> Douglas Yung
>
>
>
> From: Yung, Douglas
> Sent: Wednesday, January 28, 2015 17:46
> To: 'llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>'
> Subject: [PATCH] Add a default constructor to work around Visual Studio compiler bug
>
>
>
> Hi, I would like to submit the following patch to include/llvm/CodeGen/TargetLoweringObjectFileImpl.h which works around a bug in the Visual Studio compiler.
>
>
>
> The bug is that the compiler does not initialize a field in the class when it creates a default constructor when it should be zero-initialized. The result is that when the field is later accessed, the value is whatever value happens to be stored there, instead of the proper zero value (false in this case). The explanation of the issue is the following:
>
>
>
> Quoting The C++11 standard section [class.base.init]p8 that "In a non-delegating constructor, if a given non-static data member or base class is not designated by a mem-initializer-id (including the case where there is no mem-initializer-list because the constructor has no ctor-initializer) and the entity is not a virtual base class of an abstract class (10.4), then
>
>
>
> - if the entity is a non-static data member that has a brace-or-equal-initializer, the entity is initialized as specified in 8.5;
>
> - otherwise, if the entity is a variant member (9.5), no initialization is performed;
>
> - otherwise, the entity is default-initialized "
>
>
>
> The lack of default-initialization is what is causing this problem if we use the Visual Studio compiler:
>
> At CodeGen stage, the implicit default constructor for class TargetLoweringObjectFileELF is called. Class 'TargetLoweringObjectFileELF' has a field called 'UseInitArray'. If that field is set to true, then the compiler will emit global constructors/destructors in the .init_array section. Otherwise, it will emit those symbols in the .ctors sections.
>
>
>
> The lack of a user defined default constructor for class TargetLoweringObjectFileELF means that the compiler will have to implicitly generate a default constructor with no initializer and empty body. Then, still according to the standard ( [class.base.init]p8 ) it should default-initialize field 'UseInitArray'.
>
>
>
> The Visual Studio compiler does not perform the field initialization. Hence the intermittent problem (depending on the garbage value read from memory we either think we shall emit those symbols on the .ctors section or .init_array section).
>
>
>
> To illustrate this issue, I have attached a sample file mytest2.cpp, and the assembly generated for it by the MSVC18 (Visual Studio 2013). The source defines 3 different classes A, B and C. All are identical except for the following characteristics:
>
>
>
> *         Class A does not define a default constructor and uses one generated automatically by the compiler
>
> *         Class B defines an empty default constructor
>
> *         Class C defines an empty default constructor which initializes the Boolean field to false
>
>
>
> If you examine the assembly generated by the compiler, you can see that the constructors generated for A and B area identical, and C is similar, but it contains an initialization of the Boolean field to false which A and B are both missing.
>
>
>
> This patch works around the issue by adding a default constructor to the class which explicitly initializes the field to false (the zero value).
>
>
>
> The patch also includes an LLVM test which checks that a .init_array is not generated for a non-linux ELF target.
>
>
>
> Douglas Yung
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150209/2f8db1ce/attachment.html>