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

[Jonathan Roelofs via cfe-commits]

On 11/11/15 5:01 PM, Akira Hatanaka via cfe-commits wrote:
> 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 think diag::err_drv_unsupported_opt_for_target is the most appropriate 
one for this.

>
> 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
>
>
>
> _______________________________________________ cfe-commits mailing
> list cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>

-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded