Page MenuHomePhabricator

Fix bug 8220 - llvm-config: Only keep flags starting by -I, -D & -std for --cflags, --cxxflags & --ldflags
AbandonedPublic

Authored by sylvestre.ledru on Dec 2 2018, 6:41 AM.

Details

Reviewers
tstellar
Summary

This will change the behavior of for --cflags, --cxxflags & --ldflags
Currently, a lot of projects are filtering out the results.
It has been a pain for linux distro using llvm

Diff Detail

Event Timeline

sylvestre.ledru created this revision.Dec 2 2018, 6:41 AM

Other potential options:

  • add options --cxxflags-short --cppflags-short --ldflags-short
  • or an option -- strip-extra-flags

Don't we need to leave -fno-rtti here if LLVM was complied without rtti? Also, rather than filtering out, would it be possible to explicitly add the flags that users need to successfully link/build against this install of LLVM?

Don't we need to leave -fno-rtti here if LLVM was complied without rtti? Also, rather than filtering out, would it be possible to explicitly add the flags that users need to successfully link/build against this install of LLVM?

Some projects are filtering out, some projects are filtering in - there is no standard approach currently.

This PR is suggesting indeed to filter-in, which I think is probably better. However neither me nor sylvestre knows both GCC and Clang well enough, to know if -I -D -std covers everything. If you know that -fno-rtti is necessary then yes of course it will have to be part of this PR as well.

Is this sort of logic already covered by pkg-config? It sounds like their developers would know the most about this topic. If so, perhaps in the long-run LLVM should move to that, rather than implementing its own?

I only mentioned -fno-rtti specifically because this is something that llvm-config already knows about. I also don't have knowledge of all the possible flags that might need to be filtered.

After thinking about this more, I don't think we should pass through any user specific compiler flags at all. We should limit it to -I and -D flags added by the build system or compiler flags added by llvm specific options, like LLVM_ENABLE_RTTI. I think keeping upstream simple and having package maintainers filter-in options as needed to llvm-config makes more sense, because package maintainers know the flags that they are using and are in the best position to decide which flags users should be using too.

@tstellar will you accept this patch if I remove -std?
thanks

And should I update the release notes?

@tstellar will you accept this patch if I remove -std?
thanks

I think stripping out -fno-rtti is going to be really disruptive. Here is an alternative solution I came up with: D55391.

sylvestre.ledru abandoned this revision.Dec 7 2018, 7:22 AM