Serge Guelton via llvm-dev
2020-Mar-18 11:11 UTC
[llvm-dev] Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime registration
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.
Reid Kleckner via llvm-dev
2020-Mar-18 22:17 UTC
[llvm-dev] Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime registration
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. On Wed, Mar 18, 2020 at 4:12 AM Serge Guelton via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200318/afddf466/attachment.html>
David Blaikie via llvm-dev
2020-Mar-18 22:46 UTC
[llvm-dev] Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime registration
On Wed, Mar 18, 2020 at 3:17 PM Reid Kleckner via llvm-dev < llvm-dev at 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.> > 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. > > On Wed, Mar 18, 2020 at 4:12 AM Serge Guelton via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> 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-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200318/dba00e41/attachment.html>