Page MenuHomePhabricator

[compiler-rt] Build with correct ABI (PR38025)
ClosedPublic

Authored by RVP on Feb 6 2020, 8:16 AM.

Details

Summary

This patch fixes PR38025: Wrong ABI used when building compiler-rt

Diff Detail

Event Timeline

RVP created this revision.Feb 6 2020, 8:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 8:16 AM
RVP added a comment.Feb 6 2020, 12:23 PM

Perhaps the line:
find_compiler_rt_library(builtins ... COMPILER_RT_BUILTINS_LIBRARY in config-ix.cmake can be removed. But I am not sure.

RVP added a comment.Feb 7 2020, 9:10 AM

If it can be confirmed that there is no use for COMPILER_RT_BUILTINS_LIBRARY outside of compiler-rt/CMakeLists.txt, then it can be removed and if (COMPILER_RT_USE_BUILTINS_LIBRARY) ... from config-ix.cmake removed as well. I can upload a new diff then. Please let me know...

RVP added a comment.Feb 7 2020, 9:15 AM

See my comments in bug38025 as well.

RVP added a comment.Feb 7 2020, 9:21 AM

Also I have no way of testing this out on Windows (I noticed Petr's comment that a previous attempt to fix this bug broke Windows) . Please add anyone who can do that among the reviewers.

phosek edited reviewers, added: beanz, smeenai; removed: chandlerc.Feb 7 2020, 11:25 AM
In D74133#1864219, @RVP wrote:

Also I have no way of testing this out on Windows (I noticed Petr's comment that a previous attempt to fix this bug broke Windows) . Please add anyone who can do that among the reviewers.

I've removed @chandlerc and added @beanz and @smeenai as reviewers as additional CMake build maintainers.

I have a Windows machine for testing these kind of changes, but I'm not sure if I'll be able to test this soon due to other high priority tasks that are on my plate right now.

I generally like the solution, but one concern I have is that as implemented, find_compiler_rt_library will be invoked for every library which is likely going to increase both CMake runtime and amount of output. I'd prefer if we would invoke find_compiler_rt_library only once per target. Two ways I could think to achieve that is to either (1) move these invocations to another location such as compiler-rt/cmake/config-ix.cmake and store the result in a global variable which would then be used in add_compiler_rt_runtime, or (2) cache the result value in find_compiler_rt_library and avoid re-invoking Clang if the value is already set. I'm leaning towards (1) which seems cleaner.

RVP added a comment.Feb 7 2020, 11:53 AM

I generally like the solution, but one concern I have is that as implemented, find_compiler_rt_library will be invoked for every library which is likely going to increase both CMake runtime and amount of output.

Agreed. I have noticed that in the Cmake output though the build time impact isn't much. But better to fix it while we are at it.

I'd prefer if we would invoke find_compiler_rt_library only once per target. Two ways I could think to achieve that is to either (1) move these invocations to another location such as compiler-rt/cmake/config-ix.cmake and store the result in a global variable which would then be used in add_compiler_rt_runtime, or (2) cache the result value in find_compiler_rt_library and avoid re-invoking Clang if the value is already set. I'm leaning towards (1) which seems cleaner.

(1) looks like a better solution. The current change is something I have had in our build for over a year and I uploaded it because someone else recently pinged. If I have to refactor this change, I have to allocate some time to test it properly as well (same boat as you with other higher priority stuff). Let me see...

RVP added a comment.EditedFeb 21 2020, 10:23 AM

I'd prefer if we would invoke find_compiler_rt_library only once per target. Two ways I could think to achieve that is to either (1) move these invocations to another location such as compiler-rt/cmake/config-ix.cmake and store the result in a global variable which would then be used in add_compiler_rt_runtime, or (2) cache the result value in find_compiler_rt_library and avoid re-invoking Clang if the value is already set. I'm leaning towards (1) which seems cleaner.

I looked at what it would take to do (1). The problem is that there is no global list of all known targets. Different libraries have different lists of supported targets. I thought, maybe I could just pass --target=unknown --rtlib=compiler-rt --print-libgcc-file-name to Clang and then save the output path in a global variable. Then for a given library name and for a given target, I could replace unknown in the library path with the desired target (just like builtins is currently replaced with library name) and check if that file exists. But unfortunately, it is not so simple. For example, the target name used in CMake could be ppc64 and Clang returns powerpc64 for the target part in the library path with --target=ppc64. So replacing unknown with ppc64 in the library path would not result in the desired path. To handle this, I will have to keep this mapping (ppc64 -> powerpc64) somewhere, duplicating what Clang is already doing. This seems to be not such a clean solution anymore. Wonder if I should instead pursue solution (2) of caching the result value and avoid re-invoking Clang. The advantage is that adding a new target wouldn't need any changes to find_compiler_rt function in CMake. Any thoughts?

RVP updated this revision to Diff 246624.EditedFeb 25 2020, 9:04 PM

Implemented caching in find_compiler_rt_library to avoid re-invocation of clang.

RVP marked an inline comment as done.Feb 27 2020, 9:49 AM

@phosek @smeenai or @beanz, Can one of you please review this?

lebedev.ri retitled this revision from Fix for PR38025 to [compiler-rt] Build with correct ABI (PR38025).Feb 29 2020, 11:12 AM
lebedev.ri edited the summary of this revision. (Show Details)
phosek added inline comments.Feb 29 2020, 12:08 PM
compiler-rt/cmake/Modules/HandleCompilerRT.cmake
4

I'd consider just inlining this, I don't think a bit of code deduplication is worth the extra complexity and I find the current version more difficult to follow.

7

Either COMPILER_RT_LIBRARY_${name}_${target}-NOTFOUND or NOTFOUND is more idiomatic in CMake.

30–41

I'd use if(NOT DEFINED COMPILER_RT_LIBRARY_builtins_${target}) as a check here to differentiate the case when variable was set from the case when variable is considered to be false by CMake.

RVP marked 3 inline comments as done.Mar 1 2020, 9:03 AM
RVP added inline comments.
compiler-rt/cmake/Modules/HandleCompilerRT.cmake
4

I am not sure what is making it difficult to follow. Is it that the macro contains a return()? What if we turned the macro into a function check_if_compiler_rt_library_exists that sets the ...-NOTFOUND variable and inlined the return() into find_compiler_rt_library? That way, we won't need to duplicate all that code. I have no problem changing it the way you suggest, if you feel strongly that duplicating the code actually makes it easier to understand (but I think it is better not to duplicate).

7

Will do this.

30–41

Will do.

RVP updated this revision to Diff 247516.Mar 1 2020, 1:09 PM

Incorporates most of the changes suggested by @phosek. Have not inlined the code as suggested by @phosek but the new code seems much clearer and avoids the code duplication from inlining. Specifically, the new diff replaces the previous macro with a function. Earlier, the macro contained a return() that was presumably difficult to follow. The cache variable is now called COMPILER_RT_LIBRARY-${name}-${target} and is set to either the actual path or NOTFOUND. Also, the new diff uses if(NOT DEFINED .,.,) to distinguish when the cache variable has not been set versus when the variable is set to NOTFOUND.

RVP updated this revision to Diff 247541.Mar 1 2020, 7:26 PM
RVP marked 4 inline comments as done.

Minor fix in previous revision. Rather than return without setting ${variable}, set to NOTFOUND.

RVP marked an inline comment as done.Mar 1 2020, 7:37 PM
RVP added inline comments.
compiler-rt/cmake/Modules/HandleCompilerRT.cmake
51

If builtins is NOTFOUND, no point proceeding further. Set ${variable} to NOTFOUND and return.

RVP updated this revision to Diff 248876.Mar 6 2020, 6:19 PM

Reinstated the call to find_compiler_rt_library in compiler-rt/cmake/config-ix.cmake. Previous diff had removed this assuming that the variable COMPILER_RT_BUILTINS_LIBRARY is unused. But it seems that there are some uses of it that I missed. In this call, target is passed "" so that the default target is chosen.

RVP added a comment.EditedMar 6 2020, 6:21 PM

@phosek @smeenai @beanz -- please review. TIA.

BTW, these changes need to be verified on Windows. I don't have a Windows setup. So I will need some help with that,

This makes sense to me. I'd prefer @phosek to take another look since he has the most context here, but if he hasn't gone to it in a few days I'd be happy to accept it. Sorry for the delay here.

As for testing on Windows, it'd be fine to commit this and then monitor the Windows bots. (We also have pre-commit testing now, although I don't know if the Windows sanitizer bots are covered by that.)

Do you have commit access, or would you need someone to commit this for you?

compiler-rt/cmake/Modules/HandleCompilerRT.cmake
62

I don't think it's necessary to cache this one. The string replacement should be pretty cheap to recompute. That would also get you down to just one call to cache_compiler_rt_library, at which point you could just inline it.

RVP added inline comments.Mar 7 2020, 8:11 AM
compiler-rt/cmake/Modules/HandleCompilerRT.cmake
62

Note that what is being cached is not string replacement, (I know that is cheap) but the file existence check (which is not as cheap). The caching also avoids printing the message to the output log which was one concern that @phosek raised.

RVP added a comment.Mar 7 2020, 8:14 AM

Do you have commit access, or would you need someone to commit this for you?

No, I don't. I do need someone to commit this for me. Thanks.

smeenai accepted this revision.Mar 27 2020, 4:15 PM

LGTM. Sorry for the extreme delay here – this kept getting lost in my inbox. I'll commit this for you.

This revision is now accepted and ready to land.Mar 27 2020, 4:15 PM

What email do you want this committed under, and is Riyaz V Puthiyapurayil the right author name?

phosek accepted this revision.Mar 28 2020, 12:30 AM

LGTM as well, sorry about the delay.

compiler-rt/cmake/Modules/HandleCompilerRT.cmake
4

What I meant is that this function has a single if/else block where each side just prints a message and sets a variable, it's called only twice and in one of those invocations the error value is always false, so if you inline this function only one side will be duplicated. On the other hand, when reading the code, when you see call to this function you need to switch to read its body and switch back so there's some cognitive overhead. However, this is really minor and just my personal preference, I'm fine if you keep it.

RVP added a comment.Apr 2 2020, 11:12 PM

What email do you want this committed under, and is Riyaz V Puthiyapurayil the right author name?

Yup. Email: riyaz@synopsys.com.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 11:56 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
RVP added a comment.Apr 3 2020, 5:42 PM

@smeenai @phosek I got a bunch of emails about buildbot failures. Not sure if they are related to this. I am not able to access the failure logs when I click on the link in the email (says: lab.llvm.org takes too long to respond).

smeenai added a comment.EditedApr 3 2020, 5:54 PM
In D74133#1961050, @RVP wrote:

@smeenai @phosek I got a bunch of emails about buildbot failures. Not sure if they are related to this. I am not able to access the failure logs when I click on the link in the email (says: lab.llvm.org takes too long to respond).

Can you forward them to my email (look at the git commit logs for it ... it's my phabricator username AT fb.com)? I'll take a look.

It's really annoying that buildbot emails only go to the author and not the committer...