[LTO] ensure lto_codegen_debug_options() are passed on to LLVM

[thomasguest]

Summary

For debug purposes, users of the LTO library can configure LLVM using the lto_codegen_debug_options() function, for example to set option "-function-sections". Without this change, options passed in via lto_codegen_debug_options() are parsed but not applied during compilation.

I would like advice on how to write tests for this change.

It would be possible to create unit tests in llvm/unittests/LTO, but I can find no examples of unit tests written against code in the llvm/tools directory.

The alternative would be to write a lit system test against one of the tools. In this case the obvious candidate tool is llvm-lto. However this tool does not call lto_codegen_debug_options(), and I cannot find a way to validate the change in behaviour this change introduces.

[mehdi_amini]

Note: this is an interface intended for *debug*, you're not suppose to use it or rely on it in production: it has no stability guarantee (that I know of).
With this in mind, that seems fine to me.

[mehdi_amini]

tools/lto/lto.cpp

387

Why not doing this directly inside parseCodeGenDebugOptions()?

[thomasguest]

In D19015#398910, @joker.eph wrote:

Note: this is an interface intended for *debug*, you're not suppose to use it or rely on it in production: it has no stability guarantee (that I know of).
With this in mind, that seems fine to me.

Thanks for the comment @joker.eph

If I'm using the LTO.dll via the interface defined in llvm/include/llvm-c/lto.h, then the only function which allows me full control codegen configuration is lto_codegen_debug_options(). Perhaps the function is misnamed, and should actually be called lto_codegen_set_options().

The original purpose of this API function is for debugging, but without the fix I do not think all the options passed in will be used anyway.

[mehdi_amini]

In D19015#399459, @thomasguest wrote:

In D19015#398910, @joker.eph wrote:

Note: this is an interface intended for *debug*, you're not suppose to use it or rely on it in production: it has no stability guarantee (that I know of).
With this in mind, that seems fine to me.

Thanks for the comment @joker.eph

If I'm using the LTO.dll via the interface defined in llvm/include/llvm-c/lto.h, then the only function which allows me full control codegen configuration is lto_codegen_debug_options(). Perhaps the function is misnamed, and should actually be called lto_codegen_set_options().

No, really no...

The original purpose of this API function is for debugging, but without the fix I do not think all the options passed in will be used anyway.

The things is: throughout LLVM it is considered that any global cl::opt is here only for development and should not be used to control LLVM by a client. If there is a need to control a cl::opt other than debugging something, then there is a missing API.
For instance you are mentioning function-sections, here is how clang initialize it: https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/BackendUtil.cpp#L573

[mehdi_amini]

To put it differently: there is no command line option for LLVM (as a library), only APIs.

[thomasguest]

In D19015#399462, @joker.eph wrote:

To put it differently: there is no command line option for LLVM (as a library), only APIs.

Yes, what I am really after is a way to configure codegen when using LLVM via the LTO library. At the moment I cannot do so.

The lto_codegen_debug_options() function does allow me to pass options in and does result in these options being parsed using cl::ParseCommandLineOptions() -- the problem is, without this patch, these options do not get applied, which I think is an issue for anyone trying to debug LTO.dll using lto_codegen_debug_options().

[mehdi_amini]

In D19015#399466, @thomasguest wrote:

In D19015#399462, @joker.eph wrote:

To put it differently: there is no command line option for LLVM (as a library), only APIs.

Yes, what I am really after is a way to configure codegen when using LLVM via the LTO library. At the moment I cannot do so.

Yes, apparently no-one had this need till now, submit a patch :)

The lto_codegen_debug_options() function does allow me to pass options in and does result in these options being parsed using cl::ParseCommandLineOptions() -- the problem is, without this patch, these options do not get applied, which I think is an issue for anyone trying to debug LTO.dll using lto_codegen_debug_options().

Sure.

[thomasguest]

In D19015#399467, @joker.eph wrote:

In D19015#399466, @thomasguest wrote:

In D19015#399462, @joker.eph wrote:

To put it differently: there is no command line option for LLVM (as a library), only APIs.

Yes, what I am really after is a way to configure codegen when using LLVM via the LTO library. At the moment I cannot do so.

Yes, apparently no-one had this need till now, submit a patch :)

OK, I can work on that as a separate patch.

The lto_codegen_debug_options() function does allow me to pass options in and does result in these options being parsed using cl::ParseCommandLineOptions() -- the problem is, without this patch, these options do not get applied, which I think is an issue for anyone trying to debug LTO.dll using lto_codegen_debug_options().

Sure.

Given this, I think the patch stands as is. I have changed the title and description to reflect the new intent.

tools/lto/lto.cpp

387

I think this is a good suggestion, thanks, and would make it possible to unit test the change.

[probinson]

In D19015#399467, @joker.eph wrote:

In D19015#399466, @thomasguest wrote:

In D19015#399462, @joker.eph wrote:

To put it differently: there is no command line option for LLVM (as a library), only APIs.

Yes, what I am really after is a way to configure codegen when using LLVM via the LTO library. At the moment I cannot do so.

Yes, apparently no-one had this need till now, submit a patch :)

Let me point out that the gold plugin works exactly this way (pass in cl options as a string).
It might not be ideal but it's what we have today.
LLVM-as-library doesn't really have another way to conjure up TargetOptions (I *think* clang creates one directly, sorry can't check right now, but that seems even more fragile).

[thomasguest]

In D19015#399821, @probinson wrote:

Let me point out that the gold plugin works exactly this way (pass in cl options as a string).
It might not be ideal but it's what we have today.
LLVM-as-library doesn't really have another way to conjure up TargetOptions (I *think* clang creates one directly, sorry can't check right now, but that seems even more fragile).

Thanks for the information @probinson
I was imagining the way to configure codegen via the C API would be to add a new function: lto_codegen_configure(lto_codegen_t cg, char ** argv, int argc);
However, I think this patch still stands as a fix for debugging the LTO DLL.

[mehdi_amini]

Thanks for the information @probinson
I was imagining the way to configure codegen via the C API would be to add a new function: lto_codegen_configure(lto_codegen_t cg, char ** argv, int argc);

You got me wrong: the problem is not with the name of the API, it is with the fact that we don't want *any command line* argument, i.e. we should never call cl::ParseCommandLineOptions as part of a normal flow (i.e. other than debug).

[probinson]

In D19015#400001, @joker.eph wrote:

Thanks for the information @probinson
I was imagining the way to configure codegen via the C API would be to add a new function: lto_codegen_configure(lto_codegen_t cg, char ** argv, int argc);

You got me wrong: the problem is not with the name of the API, it is with the fact that we don't want *any command line* argument, i.e. we should never call cl::ParseCommandLineOptions as part of a normal flow (i.e. other than debug).

Passing in string options is not the objection that I understood (from previous this-is-a-library discussion). The problem is that cl::opt parses them *using static data* and that's not good for a library. Parsing string options to an API that then implemented the parsing without using static data would be fine.

[mehdi_amini]

In D19015#400002, @probinson wrote:

In D19015#400001, @joker.eph wrote:

Thanks for the information @probinson
I was imagining the way to configure codegen via the C API would be to add a new function: lto_codegen_configure(lto_codegen_t cg, char ** argv, int argc);

You got me wrong: the problem is not with the name of the API, it is with the fact that we don't want *any command line* argument, i.e. we should never call cl::ParseCommandLineOptions as part of a normal flow (i.e. other than debug).

Passing in string options is not the objection that I understood (from previous this-is-a-library discussion). The problem is that cl::opt parses them *using static data* and that's not good for a library. Parsing string options to an API that then implemented the parsing without using static data would be fine.

You're definitively right. The objection this is about cl::opt, which is what I referred to by "command line options" (the argv/argc in the proposed prototype lead me to believe that the intention was to call cl::ParseCommandLineOptions()).
I'm not fond of string based APIs but I can live with them :)

tools/lto/lto.cpp

void lto_codegen_add_must_preserve_symbol(lto_code_gen_t cg,
                                           const char *symbol) {
   unwrap(cg)->addMustPreserveSymbol(symbol);
 }
 
 static void maybeParseOptions(lto_code_gen_t cg) {
   if (!parsedOptions) {
     unwrap(cg)->parseCodeGenDebugOptions();
     lto_add_attrs(cg);
+    TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
+    unwrap(cg)->setTargetOptions(Options);
     parsedOptions = true;
   }
 }
 
 bool lto_codegen_write_merged_modules(lto_code_gen_t cg, const char *path) {
   maybeParseOptions(cg);
   return !unwrap(cg)->writeMergedModules(path);
 }