compor / llvm-ir-cmake-utils

LLVM IR CMake utils for bitcode file manipulation by opt and friends
GNU Lesser General Public License v2.1
68 stars 14 forks source link

Adding source code files from linked libraries #25

Closed tandf closed 1 year ago

tandf commented 1 year ago

Hi compor,

Regarding #23, I think you have a point. I didn't know that it's possible to use llvmir_attach_bc_targets to the library target first. Thanks for the information!

I'm building an automatic pipeline to generate IR files for a lot of cmake projects. What I have in my script is to find all targets defined using add_excutable() and add llvmir_attach_bc_targets(), llvmir_attach_link_target() and llvmir_attach_disassemble_target() for them. So for the libraries, do you think I should add an additional llvmir_attach_bc_targets() and then include it in llvmir_attach_link_target()? Thank you!

compor commented 1 year ago

Hi @tandf and thanks for elaborating on your use case.

I see what you're trying to do; it can become cumbersome to manually trace down the dependencies.

My suggestion would be to add this functionality as a target option that is by default OFF (so that it does not change the default current functionality). If that option is turned upon attaching on a target, then it would behave as you described in #23.

What do you think? Does this make sense? We can discuss this further and I'd be happy to provide feedback on a PR for this.

tandf commented 1 year ago

I think adding this functionality as a target option is a good idea. I can try to implement it, and we can discuss. Do you think that's ok?

compor commented 1 year ago

That sounds good, I'll try to prepare some pointers on how to go about it once time allows, so bare with me.

compor commented 1 year ago

Hey @tandf, the way I imagine this, is to implement it as an option using cmake_parse_arguments which we already use in llvmir_attach_bc_target.

Ideally, this behaviour could be abstracted in a separate function if it is too long, otherwise, it can just be added to that function.

One concern is what happens if two or more targets are dependent on the same target (i.e., a common library) and both set the proposed option ON.

tandf commented 1 year ago

Thanks! I'll implement it as you said soon.

tandf commented 1 year ago

Hi @compor, I have implemented a version. I use cmake_parse_arguements as you said, and only for static libraries, since they are also included in the final executable of the normal compile pipeline. I also add a commit to demonstrate the usage of this feature in example01. Could you help me take a look when you have time? Thanks!

compor commented 1 year ago

Will do it as soon as time allows. Thanks

tandf commented 1 year ago

Hi @compor

Really sorry for the late reply. Have been very busy with something else. I have modified the option name as well as the cmake message contents as you suggested.

For your concern

One thing that I'd like also to be checked is what happens if the option is ON and the dependent library is also explicitly attached to with llvm_ir_attach_bc_target. Do we get the files twice as expected?

In line 138 there is a list(REMOVE_DUPLICATES IN_FILES) which should remove the duplicate files in IN_FILES. I also left a comment there.

Please tell me if you have any questions or if there is anything else I can do.

compor commented 1 year ago

Hi @tandf, no problem at all. Thanks for the patches. Let me know how this works and if you need more in the future.

Addressed by #23