Fix multilib selection process #367
Conversation
| return; | ||
| Multilib::flags_list Flags = TC.getMultilibFlags(Args); | ||
| + if (TC.getRTTIMode() == ToolChain::RTTIMode::RM_Disabled) { | ||
| + Flags.push_back("-fno-exceptions"); |
There was a problem hiding this comment.
I believe exceptions and RTTI can be enabled/disabled independently - we can choose to combine as a library variant, but we should not hardcode them here, I think.
There was a problem hiding this comment.
When exceptions are enabled without rtti one of the tests fails. This is the reason we have it always enabled/disabled at the same time. More to say we ship only two types of library. Possibility to enable/disable rtti and exceptions independently would require to ship more library variants.
There was a problem hiding this comment.
One thing is what we do and the other is what other users of the upstream code will do. Even if there are dependencies between the two elsewhere, I agree the checks should be separate here.
Could we also have explicit flags for the positive case? I mean -fexceptions and -frtti?
As the current version works for us, I think it can be fixed during upstream review.
There was a problem hiding this comment.
First we need to decide if want to support all possible combinations of rtti and exceptions.
If we will support more versions then it makes sense to explcitly add -fexceptions to multilib flags as it would simplify selection of proper variant.
As I said there is one "exception" test that require rtti:
| LLVM-embedded-toolchain-for-Arm/repos/llvm-project/libcxx/test/std/language.support/support.exception/except.nested/throw_with_nested.pass.cpp:93:26: error: use of dynamic_cast requires -frtti
| 93 | const B& b = dynamic_cast<const B&>(e);
For me this is strong indication to use exceptions and rtti at same tame and do not allow "mixed" combinations.
There was a problem hiding this comment.
We will have one set of libraries for now, but this should be addressed in the multilib mapping file - the driver should allow splitting them later, i.e. should allow configuring exceptions and rtti flags independently.
There was a problem hiding this comment.
As for the test. I think the test should be fixed upstream with something like:
// REQUIRES: rtti
| Subject: [PATCH 15/15] Multilib support for libraries with exceptions | ||
|
|
||
| --- | ||
| clang/lib/Driver/ToolChains/BareMetal.cpp | 4 ++++ |
There was a problem hiding this comment.
Should we fix in this upstream, indead of adding a patch?
There was a problem hiding this comment.
Yes we can do it.
| return; | ||
| Multilib::flags_list Flags = TC.getMultilibFlags(Args); | ||
| + if (TC.getRTTIMode() == ToolChain::RTTIMode::RM_Disabled) { | ||
| + Flags.push_back("-fno-exceptions"); |
There was a problem hiding this comment.
One thing is what we do and the other is what other users of the upstream code will do. Even if there are dependencies between the two elsewhere, I agree the checks should be separate here.
Could we also have explicit flags for the positive case? I mean -fexceptions and -frtti?
As the current version works for us, I think it can be fixed during upstream review.
Fix multilib selection process so process will respect -fno-exception and -fno-rtti multilib flags
Fix multilib selection process
so process will respect
-fno-exception and -fno-rtti multilib flags