Page MenuHomePhabricator

Thumb state not being passed through to LLVM triple when using clang -cc1as
ClosedPublic

Authored by labrinea on Oct 27 2015, 8:20 AM.

Details

Summary

When running clang with an arm triple such as '--target=thumbv7m-arm-none-eabi' that has a thumb only CPU by default (cortex-m3), and when using the assembler, the default thumb state of the CPU does not get passed via the triple to LLVM:

$ clang -target thumbv7m-arm-none-eabi -c -v test.s
clang -cc1as ... -triple armv7m-arm-none-eabi ... test.s

Diff Detail

Repository
rL LLVM

Event Timeline

labrinea updated this revision to Diff 38547.Oct 27 2015, 8:20 AM
labrinea retitled this revision from to Thumb state not being passed through to LLVM triple when using clang -cc1as.
labrinea updated this object.
labrinea added reviewers: rengolin, cfe-commits, bsmith.
t.p.northover accepted this revision.Oct 27 2015, 9:06 AM
t.p.northover added a reviewer: t.p.northover.
t.p.northover added a subscriber: t.p.northover.

This looks mostly fine, just one quick question that shouldn't block anything:

test/Driver/arm-ias-Wa.s
67 ↗(On Diff #38547)

Is the -arm- vendor intentional (because DS-5 uses that or something)? Fine if so (it could probably do with being tested actually), a bit weird otherwise.

This revision is now accepted and ready to land.Oct 27 2015, 9:06 AM
rengolin added inline comments.Oct 27 2015, 9:11 AM
test/Driver/arm-ias-Wa.s
67 ↗(On Diff #38547)

No, that seems a typo. I don't think it should stay.

labrinea updated this revision to Diff 38552.Oct 27 2015, 9:22 AM
labrinea edited edge metadata.
rengolin accepted this revision.Oct 27 2015, 9:24 AM
rengolin edited edge metadata.

I wish there was a way to get the info if a target is thumb-only, but this is ok as an intermediate solution. :)

LGTM too, thanks!

labrinea edited edge metadata.Oct 27 2015, 9:39 AM
labrinea added a subscriber: olista01.
labrinea added inline comments.
lib/Driver/ToolChain.cpp
485 ↗(On Diff #38552)

Alternatively we could remove the check of InputType. I am not sure why this and the above comment are present anyway.

rengolin added inline comments.Oct 27 2015, 9:59 AM
lib/Driver/ToolChain.cpp
485 ↗(On Diff #38552)

I agree with you, this logic is inconsistent.

Historically, ARM assembly (especially inline assembly) is assumed ARM unless told otherwise. This is why you have to specify ".thumb" and not ".arm". However, that doesn't mean the assembly file *has* to be ARM, even if the arch name is "thumbvN" or if "-mthumb" is specified.

Not even GNU tools behave in such odd ways, so I think we should be consistent. If the triple is thumb, asm is thumb. If we have -mthumb, asm is thumb. If not, it's arm. Precisely how everything else works, so it's only logical that we should remove the InputType check.

t.p.northover added inline comments.Oct 27 2015, 10:13 AM
lib/Driver/ToolChain.cpp
485 ↗(On Diff #38552)

I suspect the issue is with our weirdness. When I remove the TY_PP_Asm check "clang -arch armv7 -c tmp.s" defaults to Thumb-mode. I think I'd be lynched by the OS-folks if that changed.

(That's basically the entire content of the Radar Chad referred to in his original addition).

rengolin added inline comments.Oct 27 2015, 10:29 AM
lib/Driver/ToolChain.cpp
485 ↗(On Diff #38552)

Well, that's clearly a bug. I'm ok with this going is as it is, but would be good to create a new bug to solve this mess.

labrinea updated this revision to Diff 38558.Oct 27 2015, 10:31 AM

It seems I missed your comments before updating the patch.
@t.p.northover: What is the exact command? "clang -arch armv7 -c tmp.s" didn't work for me.

rengolin added inline comments.Oct 27 2015, 10:44 AM
lib/Driver/ToolChain.cpp
472 ↗(On Diff #38558)

You could cache the profile and use it here, too.

test/Driver/arm-ias-Wa.s
75 ↗(On Diff #38558)

You should also add "armv7m" and check that it defaults to Thumb, no?

Is this revision valid after all? I am confused by Tim's comment. I did not see any regressions locally.

lib/Driver/ToolChain.cpp
472 ↗(On Diff #38558)

I don't see any checks based on profile in this line.

test/Driver/arm-ias-Wa.s
75 ↗(On Diff #38558)

It does default to thumb, if we are happy with this check I can add it.

If you're on Linux or something you need "clang -target x86_64-apple-darwin -arch armv7 -c tmp.s". Another mess I keep meaning to fix.

I suspect the reason for this hack is that we've already changed the triple to "thumbv7-apple-iosN" by this point (because -arch armv7 compiles to Thumb), so it needs undoing for .s files. It might be reasonably easy to push the TY_PP_Asm check back into the Darwin codepath, or it might be horrible. So much has changed since 2011.

If you're on Linux or something you need "clang -target x86_64-apple-darwin -arch armv7 -c tmp.s".

x86_64 + ARMv7? This doesn't make sense... What is this trying to achieve?

I suspect the reason for this hack is that we've already changed the triple to "thumbv7-apple-iosN" by this point (because -arch armv7 compiles to Thumb), so it needs undoing for .s files. It might be reasonably easy to push the TY_PP_Asm check back into the Darwin codepath, or it might be horrible. So much has changed since 2011.

I think so. If Darwin is special, in which it treats "armv7" as Thumb, then it needs special treatment, not the other way around.

Also, Alexandros, it would be good to add the same set of tests for Darwin, to make sure we're not messing up their side.

cheers,
--renato

lib/Driver/ToolChain.cpp
472 ↗(On Diff #38558)

Sorry, not profile, but ARMTargetParser function.

You have just replaced:

Suffix.startswith("v6m") || Suffix.startswith("v7m") || Suffix.startswith("v7em")

with:

ARM::parseArchProfile(Suffix) == ARM::PK_M

You should also replace:

Suffix.startswith("v7")

with:

ARM::parseArchVersion(Suffix) == 7
test/Driver/arm-ias-Wa.s
75 ↗(On Diff #38558)

That will give people looking at your commit in the future the idea of what you were trying to achieve.

labrinea added a comment.EditedOct 27 2015, 11:08 AM

If you're on Linux or something you need "clang -target x86_64-apple-darwin -arch armv7 -c tmp.s". Another mess I keep meaning to fix.

I suspect the reason for this hack is that we've already changed the triple to "thumbv7-apple-iosN" by this point (because -arch armv7 compiles to Thumb), so it needs undoing for .s files. It might be reasonably easy to push the TY_PP_Asm check back into the Darwin codepath, or it might be horrible. So much has changed since 2011.

I just tried that and I get :
-cc1 -triple thumbv7-apple-ios5.0.0
-cc1as -triple thumbv7-apple-ios5.0.0
which means you were right.

My previous revision gives:
-cc1 -triple thumbv7-apple-ios5.0.0
-cc1as -triple armv7-apple-ios5.0.0
so I think it should be ok.

labrinea updated this revision to Diff 38633.Oct 28 2015, 1:25 AM

I think the patch is now in it's final shape:

  • kept TY_PP_Asm check
  • added darwin and armv7m in checks in tests
  • cached IsMProfile
  • used ARM::parseArchVersion

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.