Page MenuHomePhabricator

[Driver] Delete -mimplicit-it=
ClosedPublic

Authored by MaskRay on May 15 2021, 10:22 PM.

Details

Summary

This is a GNU as and Clang cc1as option, not a GCC option.
Users should specify -Wa,-mimplicit-it= instead.

Note: mixing the -m option and the -Wa, option doesn't work
-Wa,-mimplicit-it=never -mimplicit-it=always =>
clang (LLVM option parsing): for the --arm-implicit-it option: may only occur zero or one times!

Diff Detail

Event Timeline

MaskRay created this revision.May 15 2021, 10:22 PM
MaskRay requested review of this revision.May 15 2021, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2021, 10:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers accepted this revision.May 17 2021, 11:01 AM

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.

This revision is now accepted and ready to land.May 17 2021, 11:01 AM
This revision was landed with ongoing or failed builds.May 18 2021, 10:57 AM
This revision was automatically updated to reflect the committed changes.

I would request that this commit should be reverted.

This is a GNU as

This is true

and Clang cc1as option, not a GCC option. Users should specify -Wa,-mimplicit-it= instead.

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.

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.

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:
https://code.videolan.org/videolan/x264/-/blob/b684ebe04a6f80f8207a57940a1fa00e25274f81/configure#L855
https://chromium.googlesource.com/webm/libvpx/+/66c1ff6850fd53bcf5c17247569bea1d700d6247/build/make/configure.sh#1012
https://github.com/cisco/openh264/blob/089d6c6d83ab6e5d451ef18808bb6c46faf553a2/build/platform-mingw_nt.mk#L23

Should be an easy fix either way (replace -mimplicit-it=always with -Wa,-wmimplicit-it=always).

No, it's not an easy fix as no existing releases of Clang support it.

The linux kernel already uses -Wa,-mimplicit-it. Also, GCC rejects the option:

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.)

I would request that this commit should be reverted.

Sure, @MaskRay would you mind reverting?

But this change did break my build in these places:
https://code.videolan.org/videolan/x264/-/blob/b684ebe04a6f80f8207a57940a1fa00e25274f81/configure#L855
https://chromium.googlesource.com/webm/libvpx/+/66c1ff6850fd53bcf5c17247569bea1d700d6247/build/make/configure.sh#1012
https://github.com/cisco/openh264/blob/089d6c6d83ab6e5d451ef18808bb6c46faf553a2/build/platform-mingw_nt.mk#L23

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.

Should be an easy fix either way (replace -mimplicit-it=always with -Wa,-wmimplicit-it=always).

No, it's not an easy fix as no existing releases of Clang support it.

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.

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).

Should be an easy fix either way (replace -mimplicit-it=always with -Wa,-wmimplicit-it=always).

No, it's not an easy fix as no existing releases of Clang support it.

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.

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?

MaskRay added a comment.EditedMay 19 2021, 11:41 AM

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).

Thanks for notifying the projects.

Should be an easy fix either way (replace -mimplicit-it=always with -Wa,-wmimplicit-it=always).

No, it's not an easy fix as no existing releases of Clang support it.

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.

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?

I don't mind reverting this temporarily.
However, reverting this would break musl build.
musl (since 2018-09) detects both options and will add both if available: -Wa,-mimplicit-it=never -mimplicit-it=always will cause a duplicate option failure.
Can't there be other projects which do similar detection and be broken by having both options?

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".

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".

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)

I don't mind reverting this temporarily.
However, reverting this would break musl build.
musl (since 2018-09) detects both options and will add both if available: -Wa,-mimplicit-it=never -mimplicit-it=always will cause a duplicate option failure.
Can't there be other projects which do similar detection and be broken by having both options?

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.

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".

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?

MaskRay added a comment.EditedMay 19 2021, 12:00 PM

I think libvpx's ASFLAGS usage is about GNU as, not the driver option.

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".

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)

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 ~ ).
There can be other projects doing detection on both -mimplicit-it and -Wa,-mimplicit-it and will be broken by reverting this patch.

I think libvpx's ASFLAGS usage is about GNU as, not the driver option.

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*)

I think libvpx's ASFLAGS usage is about GNU as, not the driver option.

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".

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)

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 ~ ).
There can be other projects doing detection on both -mimplicit-it and -Wa,-mimplicit-it and will be broken by reverting this patch.

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.

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.

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".
We can just assume having different values is undefined behavior. (No project should specify different values.)

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".

I said this because this would break musl arm build.

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?

If you can make -mimplicit-it= -Wa,-mimplicit-it= work, keeping the driver option bit longer LG.

I think libvpx's ASFLAGS usage is about GNU as, not the driver option.

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".

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)

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 ~ ).
There can be other projects doing detection on both -mimplicit-it and -Wa,-mimplicit-it and will be broken by reverting this patch.

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?

musl's arm build has always been working with clang. It stopped working after the driver option -mimplicit-it= was added earlier this year.
This patch dropped the driver option/fixed the musl build but broke some mingw arm projects.

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.

I think libvpx's ASFLAGS usage is about GNU as, not the driver option.

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".

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)

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 ~ ).
There can be other projects doing detection on both -mimplicit-it and -Wa,-mimplicit-it and will be broken by reverting this patch.

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?

musl's arm build has always been working with clang. It stopped working after the driver option -mimplicit-it= was added earlier this year.
This patch dropped the driver option/fixed the musl build but broke some mingw arm projects.

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.

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.

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".
We can just assume having different values is undefined behavior. (No project should specify different values.)

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.

MaskRay added a comment.EditedMay 19 2021, 12:38 PM

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).

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 ? :)

But sounds like there's some possible both-use-cases-work path forward, which is good to see.

Yes. Seems that @mstorsjo has started working on making all the use cases work.
With this, I am certainly happy that we can retain the options for a couple of releases.
(I thought musl would be broken by one major clang release but realized that we can just teach driver to ignore duplicate values so musl would be fine as well.)

mstorsjo added inline comments.May 19 2021, 2:10 PM
clang/test/Driver/arm-target-as-mimplicit-it.s
18

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.