[SYCL][Driver] Add -Z-reserved-lib-stdc++ to isObjectFile

[agozillon]

This fixes a problem that occurs when -lstdc++ and -foffload-static-lib are used together in a command.

In certain cases the -lstdc++ option is translated from the -lstdc++ string into an option by the Driver. This means it's not picked up by the isObjectFile check and gets passed down into the OffloadBundler's ConstructJobMultipleOutputs call, it will then get pushed into the arguments to the linkers job construction as a string. As it's not a string it's an Input (llvm::Opt) it will result in some garbage data as an argument to the linker (in the default case ld). This will cause the ld job before the OffloadBundler job to fail as it can't find the garbage argument and then kill the rest of the compilation e.g:

/usr/bin/ld: cannot find �H�: No such file or directory
clang-10: error: clang-offload-bundler command failed with exit code 1 (use -v to see invocation)

This was found as part of a problem in a larger build system but I think the issue is somewhat recreate-able if you create some empty files named random.a and random.o and invoke the following command:

$SYCL_BIN/clang++ -### -std=c++11 -fsycl random.o -foffload-static-lib=random.a -o random -lstdc++

The first ld command will look something like:

"/usr/bin/ld" "-r" "-o" "/tmp/random-prelink-643ea6.o" "random.o" "����" "random.a"

Signed-off-by: Andrew Gozillon [email protected]

[agozillon]

There might be a better fix for this (if it is indeed a problem outside of my environment) that I'm unaware of that someone with more experience could supply, so I'm happy to take redirection to another fix or have someone else supply a fix in its place.

The other way of fixing this I could see that keeps the fix more local to the OffloadBundler job creation is to just check if the Option has been passed down to ConstructJobMultipleOutputs and treat it appropriately (which is in this case would likely just be skipping over it as -l options seem intentionally ignored).

There's also another reserved library treated like this; it appears to be an Apple related library (although not 100% sure) called cc_kext the option it's "translated" to is: Z_reserved_lib_cckext, I didn't add it in this case as there is no Apple support at the moment with SYCL and I can't test it on my machine. Although, I am happy to add it if this is indeed a bug/this is the right fix and people wish it added!

[Fznamznon]

@AlexeySachkov FYI. I remember you faced the problem which this patch solves.

[AGindinson]

The other way of fixing this I could see that keeps the fix more local to the OffloadBundler job
creation is to just check if the Option has been passed down to ConstructJobMultipleOutputs and
treat it appropriately (which is in this case would likely just be skipping over it as -l options seem
intentionally ignored).

I think that doing it at the level of isObjectFile is appropriate, so the approach you've chosen feels right.

[mdtoguchi]

I think these changes need to be more generalized and apply to more than just the special cased libraries. Any option that is considered a LinkerInput is parsed for isObjectFile consideration (for example try '-z relro') and ends up causing the same input garbage problem.

This is a good start, but more changes will be needed.

[agozillon]

I think these changes need to be more generalized and apply to more than just the special cased libraries. Any option that is considered a LinkerInput is parsed for isObjectFile consideration (for example try '-z relro') and ends up causing the same input garbage problem.

This is a good start, but more changes will be needed.

Ah, unfortunate that it's a larger problem! I was hoping for a simple fix in this case.. I could look into it a little deeper and try to find a better solution if you'd like, if it's not urgent (as unfortunately I have a few other tasks in the way at the moment and this problem just happened to be blocking them). Otherwise I'm happy to let someone else pick up the problem and close this or supply this as an intermediate fix in the mean time.

[mdtoguchi]

I was thinking something like this. I did some quick checks and it looks to work for the cases mentioned (Driver.cpp)

@@ -4289,8 +4300,9 @@ void Driver::BuildActions(Compilation &C, DerivedArgList &Args,
// Unbundler only handles objects.
if (auto *IA = dyn_cast(LI)) {
std::string FileName = IA->getInputArg().getAsString(Args);
- if (IA->getType() == types::TY_Object &&
- !isObjectFile(FileName))
+ if ((IA->getType() == types::TY_Object &&
+ !isObjectFile(FileName)) ||
+ IA->getInputArg().getOption().hasFlag(options::LinkerInput))
// Pass the Input along to linker.
TempLinkerInputs.push_back(LI);
else

[agozillon]

Yes, that does the trick for my use case. Awesome! Somewhat similar to @AGindinson suggestion 2), very nice. Thank you.

Happy to have this closed and replaced with a new patch or I can replace the current pending changes with the ones you have suggested, whatever is easiest for you!

[mdtoguchi]

Happy to have this closed and replaced with a new patch or I can replace the current pending changes with the ones you have suggested, whatever is easiest for you!

If it's cleaner for you, a new set of changes would work for me.

[agozillon]

Rebased and pushed the change, the new commit now has a more appropriate title and commit message than the previous commit and the pull request description and title, so if merged and squashed it should use those instead!

Although I am happy to change the title/description of this pull request or open a new pull request if that makes life easier and less confusing.

There's a few other locations where the pattern IA->getType() == types::TY_Object && !isObjectFile(FileName) is checked before aborting (addDeviceDepences), it might make sense to similarly check for linker input there (perhaps not for the InputAction as it's possibly valid to pass linker information there, but maybe in relation to the OffloadUnbundlingJobAction action). It's unrelated to the problem this solves though.

[mdtoguchi]

I'm OK with not changing the title since the commit message is good.

[mdtoguchi]

Please add '-z relro' option to the test as well to provide a unique LinkerInput option (that doesn't look like lib)

[agozillon]

I shall do this, sorry for the bother!

[AGindinson]

@agozillon, I believe squashing the commits in the way you prefer and force-pushing would be fine, and also the best way to ensure these are squashed properly.

The changes themselves look sweet this way. As far as updating other similar checks, I don't think there is a need to do this within this PR. Also, I don't quite understand a scenario where we'd run into linker input being passed to the unbundling job due to the very nature of the unbundler - please elaborate if I'm missing something.

[agozillon]

@agozillon, I believe squashing the commits in the way you prefer and force-pushing would be fine, and also the best way to ensure these are squashed properly.

The changes themselves look sweet this way. As far as updating other similar checks, I don't think there is a need to do this within this PR. Also, I don't quite understand a scenario where we'd run into linker input being passed to the unbundling job due to the very nature of the unbundler - please elaborate if I'm missing something.

There probably isn't a likely scenario (I'm not too familiar with the unbundler, haven't really had much opportunity to use it/dig into it yet). I thought the aborts were mostly there to handle cases that shouldn't be possible but might slip through by accident via some future change similar to the asserts inside of Clang, but perhaps I've misinterpreted them in this case! My comment can be disregarded then.

Edit: I'll also squash the commits and force push when I push the additional test case so that it makes life easier (I try to avoid doing it myself usually as it can make it difficult for reviewers of the pull request)

[agozillon]

Added '-z relro' to the test case and squashed the commits into 1 with a more related message/title.