Static constructors should be purged from LLVM

[Quuxplusone]

Bugzilla Link	PR11944
Status	NEW
Importance	P enhancement
Reported by	Chris Lattner (clattner@nondot.org)
Reported on	2012-02-07 19:42:59 -0800
Last modified on	2019-05-20 11:07:01 -0700
Version	1.0
Hardware	PC All
CC	anton@korobeynikov.info, benny.kra@gmail.com, dblaikie@gmail.com, geek4civic@gmail.com, gm4cheng@gmail.com, jonathan.sauer@gmx.de, jryans@gmail.com, llvm-bugs@lists.llvm.org, rafael@espindo.la, samsonov@google.com, stoklund@2pi.dk, yuanfang.chen@sony.com
Fixed by commit(s)
Attachments	errors.log (597863 bytes, text/plain) errors_new.log (683047 bytes, text/plain)
Blocks
Blocked by
See also

See http://llvm.org/docs/CodingStandards.html#ci_static_ctors. We should really purge all the remaining static constructors and destructors, then turn on the clang -Wglobal-constructors warning flag to prevent regressing in the future.

[Quuxplusone]

The biggest static ctor contributor in LLVM is the cl::opt command line interface. If we want to remove static ctors from LLVM we will need a replacement for that library.

[Quuxplusone]

> The biggest static ctor contributor in LLVM is the cl::opt command line
> interface. If we want to remove static ctors from LLVM we will need a
> replacement for that library.

Any idea how we do that? Given that cl::opt is a registration system so that
parsing command line arguments can populate/use those registrations - the trick
(trivial type for the global, converting ctor for the 'real' type) done with
the global Attributes doesn't apply here - because without a real ctor the
registration would not occur.

The only other trick I know of is pinning data into particular linker sections
that could then be walked at runtime - it'd remove the static ctor but I've
only ever done something like that on Windows & not sure if/how we'd do it in a
portable way for LLVM.

Other ideas/thoughts?

[Quuxplusone]

There are a couple of ways to handle cl::opt, but they're pretty ugly and will
require some widespread API changes.  That's probably ok though, because
cl::opt really needs a redesign for performance and functionality anyway.

Anyway, I'd suggest starting with everything that is *not* cl::opt.  Last I
looked, tablegen was emitting some very large arrays that had static
constructors into each target.  There are probably a bunch of other similar
accidental ones.

[Quuxplusone]

Most of the tblgen'd madness went away during the MCTargetDesc refactoring. The remaining bit are the TargetRegisterClasses and the MVT arrays that accompany them.

The meat of it is already migrated to MC, but TargetRegisterClass itself is tricky because it contains virtual methods. Jakob wanted to fix this eventually, but I don't know if there's any short term plan.

[Quuxplusone]

The way to handle the virtual method issue is to have tblgen generate a POD struct, and then have the "virtual classes that the code generator actually forwards to" just contain the instance that tblgen poops out.

[Quuxplusone]

There are two pieces to the cl::opt refactor that I can think of. The first (easier) half of the issue it that cl::opt (and cl::list) all contain an instance of a non-pod type. A straight-forward way to fix the string case is to rework all the cl::opt stuff so that ParseCommandLineOptions makes a copy of "argv" in a BumpPointerAllocator (which is then released at llvm_shutdown time) and each cl::opt contains a StringRef that points back to these strings. Each time the "get" method is called on cl::opt, it would do an "atop" to reparse the StringRef. cl::list would be handled in a similar way, where it would actually contain an ArrayRef and the array data for the list of entries is created as ParseCommandLineOptions time, and owned by the same BumpPointerAllocator.

[Quuxplusone]

The second half of the cl::opt problem (and the more significant one) is the "static constructor to register it" problem. This dovetails (tangentially) with the problem that cl::opts are in a global namespace and that they can collide. It would be "really great" if cl::opts in the X86 target where in some x86-specific namespace and could not collide with ppc target options, for example. Similarly for mid-level optimizer passes: the loop unroll pass should just have a "threshold" cl::opt, and it would be accessible as "-loop-unroll-threshold" and the inliner would have a "threshold" cl::opt which would be accessible as "-inline-threshold".

The best way that I can think of to handle this and the registration problem together is to change cl::opt into a macro that expands into the existing global variables, as well as an initialization function for each. This initialization function would then be called by the existing pass initialization logic and would take the pass name to scope the option with. This way, the cl::opts would be initialized iff the passes and targets are initialized.

[Quuxplusone]

Looking at David's list (thanks!) I'd prioritize fixing these classes of issues
(since they are auto generated causing us to have a LOT of them):

build/lib/Target/X86/X86GenRegisterInfo.inc:3113:20: warning: declaration
requires a global constructor [-Wglobal-constructors]
  static const EVT GR32_NOAX_and_GR32_NOREXVTs[] = {

This should be an easy fix to use MVT::SimpleValueType.  These should never
contain unusual types that would require EVT.

build/lib/Target/X86/X86GenRegisterInfo.inc:3275:12: warning: declaration
requires a global constructor [-Wglobal-constructors]
  GR8Class      GR8RegClass;
                ^~~~~~~~~~~

This can be fixed by splitting the auto generated portion out into a separate
POD type generated by tblgen.

It's worth mentioning that I don't actually care about gtest static ctors, or
static ctors in llvm/tools, since they don't affect clients linking to llvm
libraries.

[Quuxplusone]

Thanks for all your thoughts/notes (& any more you provide), Chris - I'll
certainly take a look at the cases/approaches you've suggested (they haven't
quite distilled in my head yet, but I'm getting there)

> It's worth mentioning that I don't actually care about gtest static ctors, or
> static ctors in llvm/tools, since they don't affect clients linking to llvm
> libraries.

Right - we'll figure out how to turn on flags for only the right projects when
we reach that point. I'm not such a purist as to think it best to remove them
rather than bend the build system to our whim.

[Quuxplusone]

(In reply to comment #9)
> This should be an easy fix to use MVT::SimpleValueType.  These should never
> contain unusual types that would require EVT.

Done in r150173. The VT tables have hardly any users so this was surprisingly
simple.

> build/lib/Target/X86/X86GenRegisterInfo.inc:3275:12: warning: declaration
> requires a global constructor [-Wglobal-constructors]
>   GR8Class      GR8RegClass;
>                 ^~~~~~~~~~~
>
> This can be fixed by splitting the auto generated portion out into a separate
> POD type generated by tblgen.

Most of the stuff is pImpl'd into MCRegisterClass which is POD.
TargetRegisterClass itself contains a virtual method (getRawAllocationOrder)
which gets overriden by TableGen'd code. I don't see any way to do this via
value initialization. The virtual method has to be removed before this can be
fixed.

Another thing I noticed is that TargetRegisterClass also has a virtual
destructor, presumably to silence a warning that GCC emits. The destructor
should be made protected & non-virtual (the warning was fixed to accommodate
this in GCC 4.3 or 4.4) so we don't have static dtors.

[Quuxplusone]

WRT virtual dtors: LLVM is smart enough to devirtualize them here and with r150174 globalopt properly removes the calls to it, so it's not a big deal, just one extra pointer in the vtable

[Quuxplusone]

Jakob, do you have any suggestions on how we can eliminate the regclass static ctors?

[Quuxplusone]

Attached errors.log (597863 bytes, text/plain): Current -Wglobal-constructors violations

[Quuxplusone]

Attached errors_new.log (683047 bytes, text/plain): Current -Wglobal-constructors violations

[Quuxplusone]

First of all, please make sure you are not chasing windmills here. With a hot
buffer cache, a noop llc invocation runs in 0.5 ms, including static
constructors and all those pesky data relocations. I do see the problem when
linking LLVM statically into a larger application and more code needs to be
paged in, though.

Some of these data structures are very hot, so be careful to not regress LLVM
performance by adding extra indirections.

That said, I would be perfectly happy if TargetRegisterClass didn't have a type
list at all. Types have no business in codegen after isel. We only have a
couple of places that use the list, and performance isn't critical. A slightly
more expensive interface based on a SimpleTypeValue list would be fine.

We only subclass TargetRegisterClass to provide specializations of
getRawAllocationOrder(), and many classes simply use the default
implementation. I don't plan to add new virtual functions, so we could replace
the vtable pointer with a function pointer:

class TargetRegisterClass {
  ArrayRef<unsigned> (*OrderFunc)(const MachineFunction&);
  ...

  ArrayRef<unsigned> getRawAllocationOrder(const MachineFunction &MF) const {
    return OrderFunc ? OrderFunc(MF) : makeArrayRef(begin(), getNumRegs());
  }
}

Then subclassing is no longer necessary, and TargetRegisterClass can be non-
polymorphic and list-initialized.

[Quuxplusone]

TargetRegisterClasses are now value-initialized (r151806).

[Quuxplusone]

I wonder how much of these static constructors could be removed by the switch to C++11 and constexpr: Change to and make the constructor as well, and initialization happens at compile-time. Most likely this would fix tablegen issues such as those mentioned in comment 9.