[PATCH] D14471: [AArch64] Fix a crash in driver

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

In http://reviews.llvm.org/D14471#287412, @rengolin wrote:

> In http://reviews.llvm.org/D14471#286380, @ahatanak wrote:
>
> > I think I can use macro __aarch64__ to have getAArch64TargetCPU return "native" when the compiler is not run on an AArch64 platform, but it doesn't sound like that was what you had in mind?
>
>
> Not at all. :)
>
> It seems like a simple thing really, but it piles up pretty quickly.
>
> The command line:
>
>   $ clang -arch arm64 -mtune=native -c hello.c
>   
>
> doesn't make sense on an x86 machine at all. "native" means it should understand the host as an ARM64 hardware, which it's not. This should bail on error.


I agree and that is what I'm trying to do. It should error out with something like "native is not supported".

> You haven't provided any tests (you should really, what should work and what should return errors), so I can't tell just by looking at the code what you expect the new behaviour to be. I'm assuming you still want things to break, but you want your error message to have a "native" in it instead of crashing the compiler.


That's correct. The error message I'm looking for is something like the one I wrote in the summary:

error: the clang compiler does not support 'native'

I didn't include a test case because I didn't know how to write a test that passes on an aarch64 host and fails on anything else. Do you know of any test cases in trunk that pass or fail depending on which host it is running on?

> Back to the code...

> 

> Passing the pointer to a boolean is leaking logic from getAArch64TargetCPU out. If you know inside that the CPU is "native", why not just return that instead? Would that break other things? If so, these other things should be fixed. Fudging the parameters and keeping every other caller in the dark (because you're overriding the bool pointer) is certainly not the way we want to go, because that would mean two different behaviours depending on how you call the function.


Wouldn't it break on an aarch64 host? With "-mtune=native", the current code in trunk will get the host cpu name (which I believe is currently always "generic" for aarch64). But if I apply this patch, it will error out because "native" is not a valid cpu name.


http://reviews.llvm.org/D14471