[llvm-dev] Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime registration

[Serge Guelton via llvm-dev]

Hi Folks,

Commit ac1d23ed7de01fb3a18b340536842a419b504d86 introduces a change in the way
CodeGen and MC CommandFlags are handled. It's a change that may impact some
devs, so I'd better give a small notice here.

Basically previous approach was to bundle all options in a .inc file that
declares a bunch of llvm::cl options. This file was lying in include/llvm and
was to be included in client code. To avoid duplicate registration of llvm::cl
options, this was meant to be only included in binaries, and not libraries.

This rule of thumb was no respected by at least libclang-cpp and liblldCommon,
which led to situation like this:
https://bugzilla.redhat.com/show_bug.cgi?id=1756977#c5

And overall, having static options registred in « header » file is not the
panacea.

Current situation has moved to a more robust approach. options are registered
through static objects declared in the constructor of
codegen::RegisterCodeGenFlags that lies in libLLVMCodeGen. That way, when a static instance of
codegen::RegisterCodeGenFlags is created, all options are registered, and they
can only be registered once. If codegen::RegisterCodeGenFlags is not created,
the options are not registered.

Note that this approach could be used for *all* cl options.

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Reid Kleckner via llvm-dev]

This change seems at odds with past design decisions. I thought we had established a goal of reducing our reliance on static initialization.

I believe the intention of defining these flags in a header was to make it possible to link against LLVM without automatically registering them, but to allow opt and llc to share the same command line flags without code duplication. At least, that was the problem Nadav was solving when this header was originally added:

https://github.com/llvm/llvm-project/commit/e10328737db3f0e6a1a668495e4971185705d61d   

I think in practice, as indicated by the reports in the bug about lld and libclang-cpp, users actually want to make a program that behaves "just like clang", so that -mllvm flags can be parsed. There are probably only a few clients sensitive to the dynamic initialization that you have just imposed on them, but I want to bring it up, because I vaguely recall that we cared about it in the past.

Looking back, I think these options should've been provided from a real library used by opt and llc, and not just defined in a header. If this were landing today, I would suggest that these options get added as some library inside lib/Frontend instead of lib/CodeGen.

I don't personally feel strongly about adding this dynamic initialization, so I won't insist that you redesign things. I just want to try to provide some context from past discussions. You can try searching llvm-dev for "static initialization" to see some more of the history.

[David Blaikie via llvm-dev]

On Wed, Mar 18, 2020 at 3:17 PM Reid Kleckner via llvm-dev <llvm...@lists.llvm.org> wrote:

This change seems at odds with past design decisions. I thought we had established a goal of reducing our reliance on static initialization.

Seems like it /sort/ of does that - at least in that it removes namespace-scoped static storage duration declarations from LLVM library code & means only users who create a global instance of the registration object get global ctors running?

Yeah, it doesn't exactly move us towards a point where no global ctors are used, but if it's only in the executables/put next to a "main" it doesn't seem to be a bad thing, I think? (doesn't adversely effect startup time of JITs/apps that use LLVM as a library and have no need for command line flag support) But neither did the previous solution.

Not sure if this makes it less likely someone will abuse the mechanic/make the same mistake as had happened before & declare the registration object at namespace scope in a library rather than only in executables.
 

[Reid Kleckner via llvm-dev]

You know, you're right. I'm sorry, I should slow down and read more carefully. Carry on.

[Serge Guelton via llvm-dev]

On Wed, Mar 18, 2020 at 11:46 PM David Blaikie <dbla...@gmail.com> wrote:

Not sure if this makes it less likely someone will abuse the mechanic/make the same mistake as had happened before & declare the registration object at namespace scope in a library rather than only in executables.

 

If I'm not mistaken, even that should be ok. If the object is registered in a library *and* in the main executable, there will be two calls to the constructor registered in the ELF constructor. Whatever the order, the first one will initialize the static options from its constructor, and the second won't, so no conflict.