Page MenuHomePhabricator

[cmake] Strip quotes in compiler-rt/lib/crt and explicitly throw error if check executable fails
ClosedPublic

Authored by zhizhouy on Feb 24 2020, 10:31 AM.

Details

Summary

Similar change to CMakeLists as r372312.

After r372209, compiler command line may include argument with quotes:

-fprofile-instr-use="/foo/bar.profdata"

And it causes a hidden failure with execute_process later: Could not read profile "/foo/bar.profdata": No such file or directory.

In this particular case, the check for .init_array will fail silently and creates a PGO-ed binary with bad .init_array section in compiler-rt.

Bug details can be found in bug/45022

Diff Detail

Event Timeline

zhizhouy created this revision.Feb 24 2020, 10:31 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 24 2020, 10:31 AM
Herald added subscribers: llvm-commits, Restricted Project, mgorny, dberris. · View Herald Transcript
hans added a comment.Feb 25 2020, 2:15 AM

Can you provide steps to reproduce what's broken here?

zhizhouy edited the summary of this revision. (Show Details)Feb 25 2020, 10:29 AM

Created a llvm bug on how to reproduce and referring it in summary: https://bugs.llvm.org/show_bug.cgi?id=45022

Wouldn't this break when compile command contains a quoted path with spaces?

Wouldn't this break when compile command contains a quoted path with spaces?

As mentioned in the comment of r372312,
space should not be a new issue here since in line 50 we are going to replace all spaces with ";".

string(REPLACE " " ";" test_compile_command "${test_compile_command}")
xur added a comment.Feb 25 2020, 11:22 AM

Thanks for working on this. Here are a few comments:
(1) I'm not sure why PGO options are passed to compiler-rt. It's probably harmless for -fprofile-use or -fprofile-instr-use as we only use the profile. My concern is we might instrument profdata runtime routine. This can create recursion that leads to run time issue.

(2) As I mentioned in (1), passing an invalid profile does not suppose to cause error in section. Silently ignoring the option should be a noop. The error seems to be incheck_cxx_section_exists() logic. In this sense, this patch does not seem to be complete to me: For example, if we pass an invalid profile path (after stripping the "), will it still fail?

zhizhouy updated this revision to Diff 247777.EditedMar 2 2020, 6:35 PM
zhizhouy retitled this revision from [cmake] Strip quotes in compiler-rt/lib/crt to [cmake] Strip quotes in compiler-rt/lib/crt and explicitly throw error if check executable fails.

(1) I'm not sure why PGO options are passed to compiler-rt. It's probably harmless for -fprofile-use or -fprofile-instr-use as we only use the profile. My concern is we might instrument profdata runtime routine. This can create recursion that leads to run time issue.

I created a new patch to not pass PGO related options to compiler-rt: D75499

(2) As I mentioned in (1), passing an invalid profile does not suppose to cause error in section. Silently ignoring the option should be a noop. The error seems to be incheck_cxx_section_exists() logic. In this sense, this patch does not seem to be complete to me: For example, if we pass an invalid profile path (after stripping the "), will it still fail?

Seems that if the profile path is not correct, it will be filtered before getting passed into compiler-rt, so your example should not happen. But it is not good to fail silently, so I added a check for it.

xur accepted this revision.Mar 4 2020, 2:21 PM

LGTM

This revision is now accepted and ready to land.Mar 4 2020, 2:21 PM
manojgupta accepted this revision.Mar 4 2020, 2:41 PM
This revision was automatically updated to reflect the committed changes.