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

Sat Feb 7 21:27:50 PST 2015

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>:
>
> 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]
> Sent: Tuesday, February 03, 2015 19:03
> 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,
>
>
>
> 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
>:
>
> 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'
> 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
> 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>:
>
> 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]
> Sent: Tuesday, February 03, 2015 19:03
> 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,
>
>
>
> 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
>:
>
> 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'
> 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
> 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/20150208/c7e39870/attachment.html>