Page MenuHomePhabricator

[clang][driver] Only warn once about invalid -stdlib value
ClosedPublic

Authored by tbaeder on Feb 2 2021, 7:21 PM.

Details

Summary

Since ToolChain::GetCXXStdlibType() is a simple getter that might emit the "invalid library name in argument" warning, it can conceivably be called several times while initializing the build pipeline.

Before this patch, a simlpe 'clang++ -stdlib=foo ./test.cpp' would print the warning twice.

Change this and always only print the warning once. Keep the rest of the semantics of the function.

E.g, before:

$ clang++ -stdlib=foo ./test.cpp
clang-10: error: invalid library name in argument '-stdlib=foo'
clang-10: error: invalid library name in argument '-stdlib=foo'

After:

$ bin/clang++ -stdlib=foo ./test.cpp
clang-12: error: invalid library name in argument '-stdlib=foo'

Diff Detail

Event Timeline

tbaeder requested review of this revision.Feb 2 2021, 7:21 PM
tbaeder created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2021, 7:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 321022.Feb 3 2021, 12:30 AM

Noticed similar/worse behavior for -rtlib. Expanded the patch to that and -unwindlib as well.

jdoerfert added a subscriber: jdoerfert.EditedFeb 3 2021, 8:48 AM

I don't believe a static variable is a good idea. Also the name is not really informative. Members in ToolChain with a proper names would be nicer (IMHO).

Tests missing. Also, now you don't warn for different missing runtimes, which seems odd.

yaxunl added a comment.Feb 3 2021, 9:39 AM

I don't believe a static variable is a good idea. Also the name is not really informative. Members in ToolChain with a proper names would be nicer (IMHO).

Tests missing. Also, now you don't warn for different missing runtimes, which seems odd.

+1

I agree with Johannes about static variable usage. We should avoid using static variables since they are not thread safe and become hurdles when we want to parallelize the driver.

Is it possible to emit the diag in a lambda and use std::call_once with it?

Also, now you don't warn for different missing runtimes, which seems odd.

What do you mean? Is the value returned from GetRuntimeLibType() ever going to change?

My proposal would be to cache the return value of the three routines in ToolChain. This has the advantage that the values get parsed only once and there is at most one warning. I don't know how this plays with parallelization efforts, but I don't think we should worry about this right now, given the current code.

My proposal would be to cache the return value of the three routines in ToolChain. This has the advantage that the values get parsed only once and there is at most one warning. I don't know how this plays with parallelization efforts, but I don't think we should worry about this right now, given the current code.

That's what I was looking at right now as well, since using std::call_once() already means the methods can't be const anymore anyway. Might as well just cache the value. It is still slightly ugly with the current code in that subclasses can override them and then the caching is gone.

That's what I was looking at right now as well, since using std::call_once() already means the methods can't be const anymore anyway. Might as well just cache the value.

You can make the caching members mutable, IIRC that's a common pattern in Clang code.

It is still slightly ugly with the current code in that subclasses can override them and then the caching is gone.

True, but not really of concern since their implementation is usually a constant value and not emitting warnings.

tbaeder updated this revision to Diff 321349.Feb 4 2021, 1:53 AM

Here a version without the local static bools. If this is good to go, I can add some tests.

yaxunl added a comment.Feb 4 2021, 7:34 AM

LGTM. It also avoid redundant check.

Test still missing.

Hahnfeld added inline comments.Feb 4 2021, 8:36 AM
clang/include/clang/Driver/ToolChain.h
169–177

Maybe llvm::Optional for each type instead of having two members?

tbaeder updated this revision to Diff 322082.Feb 8 2021, 4:40 AM

Switched to llvm::Optional and added some tests

tbaeder marked an inline comment as done.Feb 8 2021, 4:57 AM

Is this good to go now? Or still something left? @yaxunl?

yaxunl accepted this revision.Feb 9 2021, 10:52 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Feb 9 2021, 10:52 AM

Looks like this broke the tests because -stdlib is not a thing on Windows? Would passing a linux -target x86_64-linux-gnu be an appropriate fix for the tests?