Page MenuHomePhabricator

PR4941: Add support for -fno-builtin-foo options.
ClosedPublic

Authored by mcrosier on Dec 3 2015, 10:12 AM.

Details

Summary

The changes adds support for -fno-builtin-foo options.

This addresses PR4941 and rdar://6756912.

Please take a look.

Chad

Diff Detail

Event Timeline

mcrosier updated this revision to Diff 41772.Dec 3 2015, 10:12 AM
mcrosier retitled this revision from to PR4941: Add support for -fno-builtin-foo options..
mcrosier updated this object.
mcrosier added a subscriber: cfe-commits.

Can you use a StringSet instead of a vector and avoid all (most) of the code iterating over the vector of builtins being disabled?

lib/Frontend/CompilerInvocation.cpp
147

I'd remove this; there's no need for Clang to know about all functions that the optimizer knows about, and thus for which adding the nobuiltin attribute might be useful.

Can you use a StringSet instead of a vector and avoid all (most) of the code iterating over the vector of builtins being disabled?

Hi Hal,
I began converting the code to use StringSets, but I soon realized this wasn't as trivial of a change as one would hope. There are number of places where the LangOptions and CodeGenOptions are copied (e.g., Lexer.cpp:133, PrettyPrinter.h:38, ModuleBuilder.cpp:66) . Unfortunately, this copying requires a copy constructor for the StringSets. I'd prefer to stick with a simple string vector as any code saved by not iterating over the vector is lost on the copy constructor implementation. FWIW, I also don't believe this to be performance critical code.. Please let me know your thoughts.

Chad

Chad

Can you use a StringSet instead of a vector and avoid all (most) of the code iterating over the vector of builtins being disabled?

Hi Hal,
I began converting the code to use StringSets, but I soon realized this wasn't as trivial of a change as one would hope. There are number of places where the LangOptions and CodeGenOptions are copied (e.g., Lexer.cpp:133, PrettyPrinter.h:38, ModuleBuilder.cpp:66) . Unfortunately, this copying requires a copy constructor for the StringSets. I'd prefer to stick with a simple string vector as any code saved by not iterating over the vector is lost on the copy constructor implementation. FWIW, I also don't believe this to be performance critical code.. Please let me know your thoughts.

In that case, let's stick with the vector for now. We can always improve things later if we'd like.

bob.wilson edited edge metadata.Dec 14 2015, 5:35 PM

Regarding the FIXME in lib/Frontend/CompilerInvocation.cpp: I agree with Hal that you can remove that. We used to complain about unsupported -fno-builtin-* options (and until now they have *all* been unsupported), but in r191434, Rafael changed clang to silently ignore those options, with the explanation that it matches gcc's behavior. Assuming that gcc has not changed in that regard, it makes sense that we should continue to ignore -fno-builtin-* for unsupported options.

mcrosier updated this revision to Diff 42854.Dec 15 2015, 7:43 AM
mcrosier edited edge metadata.

Remove the FIXME, per Hal and Bob's request. I confirmed that gcc does not warn for invalid -fno-builtin-foo options.
Also, pass the vector by reference and insert, rather than making an unnecessary copy on the return in CompilerInvocation.

mcrosier updated this object.Dec 18 2015, 8:53 AM
mcrosier removed reviewers: krememek, doug.gregor.

Ping. I promise this is fairly straight forward. :)

bob.wilson accepted this revision.Jan 5 2016, 9:03 AM
bob.wilson edited edge metadata.

This looks good to me. Thanks for working on this!

This revision is now accepted and ready to land.Jan 5 2016, 9:03 AM
mcrosier closed this revision.Jan 6 2016, 7:12 AM

Committed in r256937. Thanks, Bob.