Page MenuHomePhabricator

[Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64
ClosedPublic

Authored by vsk on Aug 17 2016, 10:03 PM.

Details

Summary

Fix a crash-on-invalid in which -mtune/-mcpu are set to nonsense values while -arch=arm64.

This patch extends an arm cortex test out of convenience. I'd be happy to move the test if there is a better spot for it.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 68481.Aug 17 2016, 10:03 PM
vsk retitled this revision from to [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64.
vsk updated this object.
vsk added reviewers: t.p.northover, rengolin.
vsk added a subscriber: cfe-commits.

Thanks Vedant, this also fixes the crash that occurs when -mtune=native is provided.

https://reviews.llvm.org/D14471.

lib/Driver/Tools.cpp
1163 ↗(On Diff #68481)

Is there a test case for this change? I don't think this was needed to pass the tests you added?

vsk added inline comments.Aug 18 2016, 11:17 AM
lib/Driver/Tools.cpp
1163 ↗(On Diff #68481)

Good point, I'll work up a test case.

vsk added inline comments.Aug 18 2016, 11:35 AM
lib/Driver/Tools.cpp
1163 ↗(On Diff #68481)

Actually, none of the callers of `getAArch64TargetCPU' fail when CPU="cyclone" (afaict).

Do you have a suggestion for how I can test this? I made the change here to make the function's contract more consistent, but can leave it out if you object.

ahatanak added inline comments.Aug 18 2016, 3:57 PM
lib/Driver/Tools.cpp
1163 ↗(On Diff #68481)

I couldn't find a test case for this either.

I think we should leave it out if the contract of the function is to get the string passed by either -mtune or -mcpu in "A", but we can leave it in if it's supposed to get one of the strings passed by -mtune, -mcpu, or -arch.

vsk added inline comments.Aug 18 2016, 4:00 PM
lib/Driver/Tools.cpp
1163 ↗(On Diff #68481)

All right, IMO it makes more sense to leave out -arch. I'll omit this change and update the doxygen for the function.

vsk updated this revision to Diff 68630.Aug 18 2016, 4:06 PM

Per Akira's suggestion, don't pretend that the Arg* for -arch is a user-specified CPU name (and update the doxygen to reflect this).

t.p.northover added inline comments.Aug 19 2016, 1:29 PM
test/Driver/arm-cortex-cpus.c
306 ↗(On Diff #68630)

I think these lines are relying on a Darwin host. -arch isn't a supported option on Linux & elsewhere and Clang will use its default triple. So you probably also need to specify a "-target" to put the driver into Darwin mode.

Either way, please make sure it's tested on a different platform before committing.

vsk updated this revision to Diff 68747.Aug 19 2016, 4:41 PM
  • Specify a target per Tim's comments. This now passes 'check-clang' on Ubuntu Xenial.
ahatanak accepted this revision.Sep 8 2016, 3:01 PM
ahatanak added a reviewer: ahatanak.

LGTM. We should fix this longstanding bug.

This revision is now accepted and ready to land.Sep 8 2016, 3:01 PM
This revision was automatically updated to reflect the committed changes.