Page MenuHomePhabricator

[Commandline] Move general LLVMSupport options into Commandline.cpp [NFC]
AbandonedPublic

Authored by hintonda on Dec 9 2019, 2:02 PM.

Details

Summary

This patch moves general LLVMSupport options from Debug.cpp and
Signals.cpp into CommandLine.cpp so they are only constructed if
CommandLine.o is actually loaded when statically linked. This can
help in following cases:

  1. LLVMSupport gets linked in multiple times, e.g., in a library and also in the final application, which will fail with an assert "Option 'X' registered more than once!". This is true whether or not any symbols from CommandLine.cpp are referenced in the final application, e.g., parseCommandLineOptions.
  2. Applications that don't reference any symbols from CommandLine.cpp don't have to pay for constructing unused static Options -- these general options all use cl::location, so the options are only needed for parsing. This can result in smaller binaries and faster startup times.

This patch depends on D71169.

There are other options in LLVMSupport, e.g., timing, statistics,
debug counters, etc, but they mostly used outside of LLVMSupport,
and aren't applicable to this patch.

Event Timeline

hintonda created this revision.Dec 9 2019, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 2:02 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

When you statically link in LLVMSupport, you only load what you reference. In the case of Debug options, they are referenced all over, so that TU gets loaded. If it also included static Options, which register themselves with the global parser in CommandLine.cpp, that TU gets loaded as well. By moving the Debug options over to CommandLine.cpp, those statics won't be loaded unless the TU is loaded, which will only happened if the user calls function in CommandLine.cpp.

We have a case where we use functionality in LLVMSupport, but don't call parseCommandLineOptions. We can still access the DebugFlag, etc, but don't actually need the option since it is only used by the parser -- those options use cl::location, so the global already exists. Hope that makes sense.

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

When you statically link in LLVMSupport, you only load what you reference. In the case of Debug options, they are referenced all over, so that TU gets loaded. If it also included static Options, which register themselves with the global parser in CommandLine.cpp, that TU gets loaded as well. By moving the Debug options over to CommandLine.cpp, those statics won't be loaded unless the TU is loaded, which will only happened if the user calls function in CommandLine.cpp.

We have a case where we use functionality in LLVMSupport, but don't call parseCommandLineOptions. We can still access the DebugFlag, etc, but don't actually need the option since it is only used by the parser -- those options use cl::location, so the global already exists. Hope that makes sense.

Btw, this is true for any symbol in CommandLine.cpp, not just parseCommandLineOptions. Sorry if I wasn't clear.

Thanks @hintonda for the clarification! I've added an extra question, LGTM otherwise.

llvm/lib/Support/Debug.cpp
81–82

Why did you keep that definition here?

hintonda marked an inline comment as done.Dec 11 2019, 3:28 PM
hintonda added inline comments.
llvm/lib/Support/Debug.cpp
81–82

Originally, the option was named DebugBufferSize and only used in llvm::dbgs() below.

I renamed it to DebugBufferSizeOpt when I moved to CommandLine.cpp, and added a cl::location attribute referencing the new DebugBufferSize variable. Might make sense to put it in the llvm namespace though.

hintonda updated this revision to Diff 233476.Dec 11 2019, 5:30 PM
  • Also move over DisableSymbolication option from Signals.cpp, and put all variables in llvm namespace.
hintonda retitled this revision from [Commandline] Move Debug options from Debug.cpp to Commandline.cpp to [Commandline] Move general LLVMSupport options into Commandline.cpp.Dec 11 2019, 9:14 PM
hintonda edited the summary of this revision. (Show Details)
hintonda edited the summary of this revision. (Show Details)Dec 11 2019, 9:17 PM
hintonda retitled this revision from [Commandline] Move general LLVMSupport options into Commandline.cpp to [Commandline] Move general LLVMSupport options into Commandline.cpp [NFC].Dec 12 2019, 2:57 PM

post holiday ping...

Giving some extra thoughts to that PR. I'm not a big fan of giving external visibility to flags like DisableSymbolicationFlag or DebugBufferSize. I certainly see value in activating them only on debug mode, and that could be a standalone patch.

Concerning the added value of having no options in LLVMSupport, isn't it an abstraction breakage to have debug-related stuff in the commandline implementation? It doesn't make more sense to me to have them in CL than in Support... And as you stated in the description, this doesn't solve the problem with other cl::... options in LLVMSupport.

If we want an option-less version of LLVMSupport, then I'd advocate for static variables with static initializer + getter/setter in LLVMSupport, and a separate library , say LLVMSupportCLOptions that would just use the setter/getter to properly modify LLVMSupport behavior. Just my opinion though, you probably want more insights from more people.

hintonda abandoned this revision.Apr 23 2020, 11:39 AM