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

Skip header files in llvmir_attach_bc_target #24

Closed tandf closed 1 year ago

tandf commented 1 year ago

Hi compor,

Thanks for the wonderful cmake module! It really helps me a lot.

Regarding #22, I found that some of the CMakeLists.txt files I analyze may contain something like:

add_executable(...
a.cpp
a.hpp
)

The problem is that since get_filename_component(OUTFILE ${IN_FILE} NAME_WE) is used in the LLVMIRUtil.cmake file, a.cpp and a.hpp are both regarded as target a. cmake therefore complains that there are two rules for the same target. So the first thing I did is change NAME_WE to NAME. Another thing is that because a.hpp is actually a header file, which should probably not be compiled like a source file (just my guess), I got a bunch of errors. My solution is to 1. add these header files as included files. Actually, I add their directories, which I think should do the same thing without causing trouble. 2. skip these header files when handling IN_FILES.

compor commented 1 year ago

Thanks again for elaborating.

I think the projects that list header files in an add_executable directive are not declared properly, so that is something for "them" to fix. On the llvm-ir-cmake-utils side, I'd say skipping header files and issuing a warning should do it; thus it won't need a change in in the name extraction (i.e., NAME_WE).

How does that sound?

I've reverted #22 to a draft, but if you have no further changes to make, please mark it as review-ready again and I'll have a look ASAP.

tandf commented 1 year ago

That's great! I think a warning should help people realize the cause of the problem. I have marked it as review-ready.

I still have some confusion on add_executable. Since I'm not that familiar with cmake, I cannot tell if it is legitimate to add header files in add_executable. I searched, and it seems some this is not a rare usage. From this Stack Overflow answer, adding header seems to be fine. I think their reason is that for older version of cmake, this is the only way.

As I said, I'm not familiar with cmake, so I cannot judge whether supporting header files in add_excutable is necessary. I have to leave that to you, and I'm ok with both choices.

compor commented 1 year ago

Sounds good, I'll have a look once I get some time. Thanks

tandf commented 1 year ago

Sorry for the late reply. Thanks for the comments! I have a new commit regarding the comments. Please help me review it when you have time. Thanks!

compor commented 1 year ago

Resolved by #22