Hi Kostya, One unexpected piece of fallout in your recent attributes change (r148553) was that it introduced a bunch of static constructors into .o files that #include Attributes.h, due to stuff like this: const Attributes None (0); ///< No attributes have been set const Attributes ZExt (1<<0); ///< Zero extended before/after call const Attributes SExt (1<<1); ///< Sign extended before/after call const Attributes NoReturn (1<<2); ///< Mark the function as not returning We really don't like static ctors in LLVM, because it is often used as a library, and they cause startup-time performance hits and other bad news. I'm surprised we don't have an explicit section about it in the CodingStandards, but we do have: http://llvm.org/docs/CodingStandards.html#ll_iostream ... which talks about the same thing. Anyway, as it turns out, LLVM can optimize those static ctors away in some cases, but not all (e.g. -O0). This was found because LLDB builds with -Wstatic-constructor and this header change is causing a flood of warnings. I can think of two ways to fix this. One is to replace all of these with "get" functions, which would be really really ugly. A better change would be to replace these with enums, eliminating the whole problem. Are 64-bit enums portable enough to be used here? If not, we could split this into two enums (one for attributes 0-31 and one for attributes 32+), and define the appropriate constructors in Attribute that take them, and define the appropriate operator| implementations that merge them (returning an Attribute). Can you take a look at this? It's a pretty big problem for LLDB and other clients that use -Wstatic-constructor. -Chris
I see the problem. Let me try to come up with a solution that does not involve constructors but also does not sacrifice type safety. On Tue, Feb 7, 2012 at 12:01 PM, Chris Lattner <clattner at apple.com> wrote:> Hi Kostya, > > One unexpected piece of fallout in your recent attributes change (r148553) > was that it introduced a bunch of static constructors into .o files that > #include Attributes.h, due to stuff like this: > > const Attributes None (0); ///< No attributes have been set > const Attributes ZExt (1<<0); ///< Zero extended before/after call > const Attributes SExt (1<<1); ///< Sign extended before/after call > const Attributes NoReturn (1<<2); ///< Mark the function as not returning > > We really don't like static ctors in LLVM, because it is often used as a > library, and they cause startup-time performance hits and other bad news. > I'm surprised we don't have an explicit section about it in the > CodingStandards, but we do have: > http://llvm.org/docs/CodingStandards.html#ll_iostream > > ... which talks about the same thing. > > Anyway, as it turns out, LLVM can optimize those static ctors away in some > cases, but not all (e.g. -O0). This was found because LLDB builds with > -Wstatic-constructor and this header change is causing a flood of warnings. > > > I can think of two ways to fix this. One is to replace all of these with > "get" functions, which would be really really ugly.grrrr> A better change would be to replace these with enums, eliminating the > whole problem. Are 64-bit enums portable enough to be used here? >We had a problem with 64-bit enums on the windows side, so I guess the answer is "not portable"> > If not, we could split this into two enums (one for attributes 0-31 and > one for attributes 32+), and define the appropriate constructors in > Attribute that take them, and define the appropriate operator| > implementations that merge them (returning an Attribute). > > Can you take a look at this? It's a pretty big problem for LLDB and other > clients that use -Wstatic-constructor. >--kcc> > -Chris >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120207/34d6f86c/attachment.html>
On Tue, Feb 7, 2012 at 10:01 AM, Chris Lattner <clattner at apple.com> wrote:> Hi Kostya, > > One unexpected piece of fallout in your recent attributes change (r148553) > was that it introduced a bunch of static constructors into .o files that > #include Attributes.h, due to stuff like this: > > const Attributes None (0); ///< No attributes have been set > const Attributes ZExt (1<<0); ///< Zero extended before/after call > const Attributes SExt (1<<1); ///< Sign extended before/after call > const Attributes NoReturn (1<<2); ///< Mark the function as not returning > > We really don't like static ctors in LLVM, because it is often used as a > library, and they cause startup-time performance hits and other bad news. > I'm surprised we don't have an explicit section about it in the > CodingStandards, but we do have: > http://llvm.org/docs/CodingStandards.html#ll_iostream > > ... which talks about the same thing. > > Anyway, as it turns out, LLVM can optimize those static ctors away in some > cases, but not all (e.g. -O0). This was found because LLDB builds with > -Wstatic-constructor and this header change is causing a flood of warnings. > > > I can think of two ways to fix this. One is to replace all of these with > "get" functions, which would be really really ugly. A better change would > be to replace these with enums, eliminating the whole problem. Are 64-bit > enums portable enough to be used here?I know Kostya and others are looking at this, but I have to say: why-oh-why can't we have constexpr! ;] *This* is the problem those were meant to solve, and they actually seem to solve it.... ahh well... -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120207/692cad2d/attachment.html>
Another simple solution is to use "typedef uint64_t Attributes " w/o any enums (just "const uint64_t None = 0", etc). This should work now because we already cleaned up all uses of unsigned, but someone may use unsigned instead of Attributes later. We can make such bugs easier to discover by moving half of the attribute constant to the higher 32-bits. ? --kcc On Tue, Feb 7, 2012 at 12:53 PM, Kostya Serebryany <kcc at google.com> wrote:> I see the problem. > Let me try to come up with a solution that does not involve constructors > but also does not sacrifice type safety. > > On Tue, Feb 7, 2012 at 12:01 PM, Chris Lattner <clattner at apple.com> wrote: > >> Hi Kostya, >> >> One unexpected piece of fallout in your recent attributes change >> (r148553) was that it introduced a bunch of static constructors into .o >> files that #include Attributes.h, due to stuff like this: >> >> const Attributes None (0); ///< No attributes have been set >> const Attributes ZExt (1<<0); ///< Zero extended before/after call >> const Attributes SExt (1<<1); ///< Sign extended before/after call >> const Attributes NoReturn (1<<2); ///< Mark the function as not >> returning >> >> We really don't like static ctors in LLVM, because it is often used as a >> library, and they cause startup-time performance hits and other bad news. >> I'm surprised we don't have an explicit section about it in the >> CodingStandards, but we do have: >> http://llvm.org/docs/CodingStandards.html#ll_iostream >> >> ... which talks about the same thing. >> >> Anyway, as it turns out, LLVM can optimize those static ctors away in >> some cases, but not all (e.g. -O0). This was found because LLDB builds >> with -Wstatic-constructor and this header change is causing a flood of >> warnings. >> >> >> I can think of two ways to fix this. One is to replace all of these with >> "get" functions, which would be really really ugly. > > > grrrr > >> A better change would be to replace these with enums, eliminating the >> whole problem. Are 64-bit enums portable enough to be used here? >> > > We had a problem with 64-bit enums on the windows side, so I guess the > answer is "not portable" > > >> >> If not, we could split this into two enums (one for attributes 0-31 and >> one for attributes 32+), and define the appropriate constructors in >> Attribute that take them, and define the appropriate operator| >> implementations that merge them (returning an Attribute). >> >> Can you take a look at this? It's a pretty big problem for LLDB and >> other clients that use -Wstatic-constructor. >> > > --kcc > >> >> -Chris >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120207/5eaec229/attachment.html>
On Tue, Feb 7, 2012 at 12:53 PM, Kostya Serebryany <kcc at google.com> wrote:> I see the problem. > Let me try to come up with a solution that does not involve constructors > but also does not sacrifice type safety. > > >I have a patch that uses a proxy POD type. 'make && make check' passes. It's a bit ugly in the header file (include/llvm/Attributes.h), but does not require any changes in the rest of the code, is typesef and Attributes retain the default CTOR (i.e. no uninitialized Attributes objects). If the approach is ok, I'll this patch (formatted and commented) for further review. --kcc -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120207/ebbc0eeb/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: attr.patch Type: text/x-patch Size: 7791 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120207/ebbc0eeb/attachment.bin>
> Can you take a look at this? It's a pretty big problem for LLDB and other clients that use -Wstatic-constructor.Once we clean this up - should we add -Wglobal-constructors to the LLVM build itself to avoid/highlight such regressions in the future? (a cursory build with that option on shows we do have a few violations - so perhaps it's not a universal rule - is it bad enough we should suppress it on a case-by-case basis?) - David