Differential D102568
[Driver] Delete -mimplicit-it= Authored by MaskRay on May 15 2021, 10:22 PM.
Details This is a GNU as and Clang cc1as option, not a GCC option. Note: mixing the -m option and the -Wa, option doesn't work
Diff Detail
Event TimelineComment Actions Seems fine to me; the only reference in all of Android that looks problematic might be building libaom for windows: https://android.googlesource.com/platform/external/libaom/+/refs/heads/master/libaom/build/cmake/aom_configure.cmake#158, which I don't think we do for Android. Should be an easy fix either way (replace -mimplicit-it=always with -Wa,-wmimplicit-it=always). The linux kernel already uses -Wa,-mimplicit-it. Also, GCC rejects the option: https://godbolt.org/z/8rxjY9dPj. Comment Actions I would request that this commit should be reverted.
This is true
This is not true if you look at any existing stable release - Clang 12.0 doesn't support -Wa,-mimplicit-it=. That was only added in D96285 on February 11th. I would request that you leave both forms of the option to coexist for at least a couple stable releases, so that users have a reasonable way of migrating forward, without immediately dropping support for all older versions of the compiler. It turns out that libaom actually doesn't have any standalone gas-style assembly, so that option there won't ever have any effect (otherwise it would have broken my build). But this change did break my build in these places:
No, it's not an easy fix as no existing releases of Clang support it.
Indeed. The reason why it's common in these cases, and why the difference to GCC's behaviour in these cases hasn't been noticed, is that this flag is often needed for windows/arm, and GCC doesn't support windows/arm at all, so all those codepaths have only been used with Clang. (The reason why this option often is used for that target, is that windows on arm only supports thumb - while many codecs with handwritten assembly is written to be built in arm mode only; handwritten assembly that hasn't taken thumb mode into account may need -mimplicit-it=always to work in thumb mode.) Comment Actions Sure, @MaskRay would you mind reverting?
Thanks for the links; @MaskRay I think you should file some issues in those projects to help them move to the assembler option before we try to remove it again.
Well, these projects linked above will need to implement either feature detection for the command line flag options before hard coding them, or compiler version checks. Comment Actions Well I already started doing that, I had made one PR, when CI tests there informed me that the new form of the option didn't work out with the version of Clang in use there (10.0 fwiw).
I really think it's silly to demand multiple projects to implement detection for this option. Just let's revert this change, let both option forms coexist in a couple public stable releases, until it's acceptable to raise the minimum required tool version for those build configurations to Clang 13 (it's a quite niche configuration, but upgrading CI systems always takes some time), and then after that drop the old form (e.g. in Clang 15). Maybe we could add a deprecation warning to the one that we're going to remove in the future? Comment Actions Thanks for notifying the projects.
I don't mind reverting this temporarily. I think "waiting for a few releases" is too much and doesn't improve things (they will notice issues until you remove the option). I can accept "waiting for one major release". Comment Actions
Having fairly broad windows of not breaking backwards compatibility is a fairly reasonable request. Take LLVM itself, for instance - which supports building with Clang back as far as 3.5. (not suggesting we need to have a window that large for every feature/piece of surface area - but that there is scope for fairly wide windows) Comment Actions I'm curious - if it detects both forms and adds both options, why does it add them with contradicting option values? If I understand things correctly, adding both options with matching values isn't an error? Ok, so now I checked, and it does seem to error out even if they have matching values - that sounds like the real issue to me. In the meantime, wouldn't it be possible to detect the presence of the other one and check if they match or not, to avoid passing duplicate options to the backend? I can give that a try.
Ok, that's at least some sort of middle ground. Would fixing the duplicate option issue (as long as they have matching values) open up for keeping both a little bit longer? Comment Actions I think libvpx's ASFLAGS usage is about GNU as, not the driver option. I understand the broad Windows and I also strive for this portability. However, reverting this patch is now a trade-off. It would make x264/openh264's mingw build happy but break musl's arm build (2018-09 ~ ). Comment Actions It is indeed for calling a GNU as-like tool, but at https://chromium.googlesource.com/webm/libvpx/+/66c1ff6850fd53bcf5c17247569bea1d700d6247/build/make/configure.sh#1003 it sets AS="$CC -c", which is for cases when using clang-based tools where there's no standalone as-like tool to use. My initial patch for that case looked like this: diff --git a/build/make/configure.sh b/build/make/configure.sh index 81d30a16c..526c08d3a 100644 --- a/build/make/configure.sh +++ b/build/make/configure.sh @@ -1009,7 +1009,16 @@ EOF if enabled thumb; then asm_conversion_cmd="$asm_conversion_cmd -thumb" check_add_cflags -mthumb - check_add_asflags -mthumb -mimplicit-it=always + check_add_asflags -mthumb + case ${tgt_os} in + win*) + # If $AS is $CC, this flag needs to be passed with -Wa. + check_add_asflags -Wa,-mimplicit-it=always + ;; + *) + check_add_asflags -mimplicit-it=always + ;; + esac fi ;; vs*) Comment Actions Not sure I follow - presumably musl's arm build wasn't working with clang before this patch? Or something changed there after this patch landed? It's not OK to say "this patch fixed a bug for someone, so now reverting it is on equal footing with the regressions the patch caused" - if a patch broke an existing use case that's different from fixing a use case that wasn't working to begin with & reverting to the known/existing failures would be where things tend to lean (not absolute by any means - there are cases where we tradeoff adding a new bug to fix an existing bug) if all other things are equal. Comment Actions Should be possible! Thanks for offering the help. I think it is too much to test contradicting values like // NEVER_ALWAYS: "-mllvm" "-arm-implicit-it=never" "-mllvm" "-arm-implicit-it=always".
I said this because this would break musl arm build.
If you can make -mimplicit-it= -Wa,-mimplicit-it= work, keeping the driver option bit longer LG. Comment Actions musl's arm build has always been working with clang. It stopped working after the driver option -mimplicit-it= was added earlier this year.
Comment Actions Ah, thanks for the history. Yeah, still not sure those bugs are on equal footing - if the patch had been in for a few months before musl was fixed (If they're interested in building from LLVM head - wonder why it took so long & if they aren't, then it doesn't seem like we need to rush to fix this/keep the fix in tree if backing it out and considering how to support both new-ish (self-imposed due to the -mimplicit-it= change) and existing (mingw arm) use cases can be handled gracefully and have it in the next LLVM release or the like). But sounds like there's some possible both-use-cases-work path forward, which is good to see. Comment Actions Yep, that sounds good to me. The old tests seem to be written under the assumption that it's ok to specify multiple values and the last one take effect, but that doesn't seem to be the case, not even when that patch was applied in February. I'll get started at making duplicate options with the same value work in one way or another. Comment Actions The original driver option was added in 2016-07. Some projects may accrue some -mimplicit-it usage singe then. (GCC never has the driver option but nobody had noticed the problem.) The option -Wa,-mimplicit-it was added in 2021-02. @raj.khem noticed the issue in 2021-05. Perhaps that's not too long ? :)
Yes. Seems that @mstorsjo has started working on making all the use cases work.
| ||||||
I noted another discrepancy here; the tests seem to suggest that you can pass multiple -Wa,-mimplicit-it= and only the last one will take effect, while in practice this will cause backend errors. I'll fix that at the same time.