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

[Renato Golin via cfe-commits]

rengolin added a comment.

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.

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.

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.

If the function returns a string with the CPU name, and the CPU name is "native", than logic dictates that the return value should be std::string("native");

If the function wants to add additional logic to grab the native CPU name and return that, it's extra. If it can't find the CPU name, it should return "native", not "generic" or "cyclone".

Makes sense?

cheers,
--renato


http://reviews.llvm.org/D14471