[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In D151834#4644422 <https://reviews.llvm.org/D151834#4644422>, @zahiraam wrote:

> In D151834#4644394 <https://reviews.llvm.org/D151834#4644394>, @aaron.ballman wrote:
>
>> In D151834#4644391 <https://reviews.llvm.org/D151834#4644391>, @zahiraam wrote:
>>
>>> In D151834#4644378 <https://reviews.llvm.org/D151834#4644378>, @aaron.ballman wrote:
>>>
>>>> In D151834#4644375 <https://reviews.llvm.org/D151834#4644375>, @zahiraam wrote:
>>>>
>>>>> In D151834#4644373 <https://reviews.llvm.org/D151834#4644373>, @aaron.ballman wrote:
>>>>>
>>>>>> In D151834#4643925 <https://reviews.llvm.org/D151834#4643925>, @uabelho wrote:
>>>>>>
>>>>>>> Hi @zahiraam ,
>>>>>>>
>>>>>>> I have a couple of downstream testcases that fail with this patch.
>>>>>>> Before
>>>>>>>
>>>>>>>   > clang bbi-86364.c -lm -O3
>>>>>>>   > ./a.out
>>>>>>>
>>>>>>> passed but with the patch the assert in the program fails:
>>>>>>>
>>>>>>>   a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.
>>>>>>>
>>>>>>> Is this as expected?
>>>>>>>
>>>>>>> F29200339: bbi-86364.c <https://reviews.llvm.org/F29200339>
>>>>>>
>>>>>> This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?
>>>>>
>>>>> I haven't looked at it as I saw that the comment has been deleted. Let me look into it.
>>>>
>>>> Oh, it's not the inclusion of errno.h that matters, it's the declaration of `__errno_location`: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of `__attribute__ ((__const__))` is what makes the distinction: https://godbolt.org/z/1bePhvaG4
>>>
>>> Yes confirming this. In the errno.h header the function is declared: extern int *__errno_location (void) __attribute__ ((__nothrow__ )) __attribute__ ((__const__)); 
>>> Since we can't really change the header, this needs to be changed in the compiler?
>>
>> Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.
>
> I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

This looks like it's happening only for -Oi (i>=1) and on Linux only?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151834/new/

https://reviews.llvm.org/D151834