This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple
ClosedPublic

Authored by michaelplatings on Jun 21 2023, 8:04 AM.

Details

Summary

A common user mistake is specifying a target of aarch64-none-eabi or
arm-none-elf whereas the correct names are aarch64-none-elf &
arm-none-eabi. Currently if a target of aarch64-none-eabi is specified
then the Generic_ELF toolchain is used, unlike aarch64-none-elf which
will use the BareMetal toolchain. This is unlikely to be intended by the
user so issue a warning that the target is invalid.

The target parser is liberal in what input it accepts so invalid triples
may yield behaviour that's sufficiently close to what the user intended.
Therefore invalid triples were used in many tests. This change updates
those tests to use valid triples.
One test (gnu-mcount.c) relies on the Generic_ELF toolchain behaviour so
change it to explicitly specify aarch64-unknown-none-gnu as the target.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2023, 8:04 AM
michaelplatings requested review of this revision.Jun 21 2023, 8:04 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 21 2023, 8:04 AM

Please tag the commit title with the subproject, [clang] in this case. If only to help your friendly neighborhood buildbot minder.

I'd maybe go with "Did you mean" instead of "try" but only because the first thing I think is well if I try it what will I get. Then again we're not going to fit an explanation of Arm triples into a warning message. I know elsewhere we already have "did you mean" but if "try" is already used too it's fine as is.

peter.smith added a comment.EditedJun 22 2023, 2:12 AM

I can confirm that we have seen several users of the LLVM Embedded Toolchain for Arm that work on both AArch64 and Arm make this mistake as it is easy to just alter an example triple by substituting the first element.

Given that this is a warning, invalid is perhaps a bit strong.

Perhaps "Unrecognized environment <environment> for <arch>, did you mean <arch>-none-<suggested environment>"

Perhaps "Unrecognized environment <environment> for <arch>, did you mean <arch>-none-<suggested environment>"

I like explaining it like this.

Apply @DavidSpickett's suggestions.

@peter.smith since the warning is only emitted in specific cases that we can be say with a high degree of confidence are incorrect I think it's accurate to say "invalid". I think it would be inaccurate to say "unrecognized" since the code specifically does recognise the elf and eabi* environments. Also the word invalid corresponds with the warning type (-Winvalid-command-line-argument). I thought about changing the wording to "invalid environment in target triple '...'" but I suspect most people wouldn't know that elf and eabi are "environments" and therefore the simpler message would be preferable. What do you think?

michaelplatings retitled this revision from Warn on invalid Arm or AArch64 baremetal target triple to [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple.Jun 22 2023, 5:15 AM

My initial reactions to seeing Invalid target triple aarch64-none-eabi; try aarch64-none-elf [*] were:

  • Why is it invalid?
  • I assumed it was an error message, and was about to write a comment until I saw it was a warning.
  • now did you mean (thanks for making that change).

An alternative suggestion:

<input triple> selects the fallback GCC toolchain driver; did you mean <proposed triple>

This isn't something I'm going to die on a hill for. Perhaps add some more reviewers to get some opinions, I'm happy to go with the consensus.

<input triple> selects the fallback GCC toolchain driver; did you mean <proposed triple>

This is a correct description of the current behaviour, but I want to keep the freedom to change the behaviour in future. Therefore I'd like to avoid defining what behaviour an invalid target triple will cause.

  • Why is it invalid?

Fair question. I could rephrase to:

clang: warning: mismatch between architecture and environment in target triple 'aarch64-none-eabi'; did you mean 'aarch64-none-elf'? [-Winvalid-command-line-argument]

Does that make it clearer? (I still suspect that people won't understand what is meant by "environment" here but I can't think of a better explanation).

  • I assumed it was an error message, and was about to write a comment until I saw it was a warning.

It might not be clear in the code but warning: is automatically part of the output.

clang: warning: mismatch between architecture and environment in target triple 'aarch64-none-eabi'; did you mean 'aarch64-none-elf'? [-Winvalid-command-line-argument]

That looks good to me. Would be happy with that.

Does that make it clearer? (I still suspect that people won't understand what is meant by "environment" here but I can't think of a better explanation).

"clang triple environment" into Google gets you the cross compilation guide and the Triple class docs so "environment" is useful here as one more keyword for that.

Tweak warning message

peter.smith accepted this revision.Jun 23 2023, 3:22 AM

Thanks for the update! LGTM.

This revision is now accepted and ready to land.Jun 23 2023, 3:22 AM

Thank you for implementing this warning. Side note: it would be better to (a) fix the tests separately from (b) implementing the warning and (c) adding tests to demonstrate the warning.
This way the patch is more focused.

alexfh added a subscriber: alexfh.Jun 27 2023, 4:28 PM

After the patch, LLVM still has a number of aarch64-arm-none-eabi usages in tests, which is also considered invalid now: https://gcc.godbolt.org/z/z8cY5j68M

Thank you for implementing this warning. Side note: it would be better to (a) fix the tests separately from (b) implementing the warning and (c) adding tests to demonstrate the warning.
This way the patch is more focused.

Agreed, it would have been better to fix the tests first in a separate patch.

After the patch, LLVM still has a number of aarch64-arm-none-eabi usages in tests, which is also considered invalid now: https://gcc.godbolt.org/z/z8cY5j68M

Huh. I have no idea why they didn't come up in my find-and-replace. Thanks @MaskRay for fixing. I found some more: D153943