Static ctors in llvm::Attribute

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.

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

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…

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

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

attr.patch (7.61 KB)

Slightly formatted/commented patch.
WDYT?

–kcc

attr.patch (8.24 KB)

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

This seems to work fine, except that reading a field from a const
AttrConst is not a constant expression in C++03, so the "set"
declarations (ParameterOnly, FunctionOnly, VarArgsIncompatible,
and MutuallyIncompatible) still require a global constructor. You
can split the values into separate 'const uint64_t' declarations and
use those; it makes the header even uglier, but it works.

John.

Slightly formatted/commented patch.
WDYT?

This seems to work fine, except that reading a field from a const
AttrConst is not a constant expression in C++03, so the “set”
declarations (ParameterOnly, FunctionOnly, VarArgsIncompatible,
and MutuallyIncompatible) still require a global constructor.

OMG, yes, indeed.

You
can split the values into separate ‘const uint64_t’ declarations and
use those; it makes the header even uglier, but it works.

like this (see the new patch)?

–kcc

attr.patch (8.42 KB)

Yes, that’s great, thanks!

John.

Slightly formatted/commented patch.
WDYT?

This seems to work fine, except that reading a field from a const
AttrConst is not a constant expression in C++03, so the “set”
declarations (ParameterOnly, FunctionOnly, VarArgsIncompatible,
and MutuallyIncompatible) still require a global constructor.

OMG, yes, indeed.

You
can split the values into separate ‘const uint64_t’ declarations and
use those; it makes the header even uglier, but it works.

like this (see the new patch)?

Yes, that’s great, thanks!

Shall I commit?

–kcc

Go ahead; we’ll adjust as necessary if it doesn’t fix our internal problem.

John.

Thanks Kostya!

-Chris

done, r150031.

–kcc