[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146148#4222306 <https://reviews.llvm.org/D146148#4222306>, @zahiraam wrote:

> In D146148#4221651 <https://reviews.llvm.org/D146148#4221651>, @rjmccall wrote:
>
>> In D146148#4220591 <https://reviews.llvm.org/D146148#4220591>, @zahiraam wrote:
>>
>>> In D146148#4220495 <https://reviews.llvm.org/D146148#4220495>, @aaron.ballman wrote:
>>>
>>>> In D146148#4220433 <https://reviews.llvm.org/D146148#4220433>, @zahiraam wrote:
>>>>
>>>>> In D146148#4220268 <https://reviews.llvm.org/D146148#4220268>, @rjmccall wrote:
>>>>>
>>>>>> Okay, so modifying `math.h` to use this attribute is acceptable?  That's great, that's definitely the best outcome for the compiler.
>>>>>>
>>>>>> Just a minor request about the diagnostic name, but otherwise LGTM.
>>>>>
>>>>> Yes. Added the attribute inside the included math.h header file.
>>>>
>>>> How does this work for, say, glibc, musl, or MSVC CRT? Won't those math.h headers lack the attribute and thus run into problems when used with Clang?
>>>
>>> Good point! @rjmccall are you thinking of something in particular with the attribute?
>>
>> Zahira, this is what I was asking you when I asked whether modifying the math.h header was acceptable: whether you were prepared to accept that the warning would only fire on system math.h headers that we'd modified, or whether you cared about making it work with non-cooperative headers.  I wasn't asking if you were willing to change the test code.
>
> Okay sorry about the confusion. I think the diagnostic should fire when the user is including system's math.h and using float_t inside a scope cotaining a #pragma clang fp eval-method. 
> I am not sure what you mean by "cooperative headers"?

Ones that know about the special marking and use it (cooperative) or ones that don't use the special marking (uncooperative). My intuition is that we'll want this to work with any math.h because the problem still exists if you use an older C standard library release with a newer Clang.

>>> If not I guess we will have to rely on string comparison for all the typedef in the TU. Aaron pointed out the compile time overhead.
>>
>> Well, the compile-time overhead of doing this on every typedef *declaration* is way better than the overhead of doing this on every type lookup, at least.
>>
>> Anyway, there are three possibilities I can see:
>>
>> - We accept that this needs cooperative system headers.
>
> Not sure what you mean by cooperative system headers.
>
>> - We add a math.h compiler header that `#include_next`s the system math.h and then adds the attribute.  I believe you can just add an attribute to a typedef retroactively with something like `typedef float_t float_t __attribute__((whatever))`.
>
> So when a user writes:
>
>   #include <math.h>
>    int foo()
>    {
>       #pragma clang fp eval_method(source)
>      return sizeof(float_t);
>    }
>
> It would be as though the user wrote:
>
>   #include "math.h"
>   int foo()
>   {
>      #pragma clang fp eval_method(source)
>     return sizeof(float_t);
>   }
>
> where the content of this new math header being:
>
>   #include_next <math.h>
>   typedef float_t float_t __attribute__((whatever));
>   typedef double_t double_t __attribute__((whatever));

Correct.

> The compiler would have to recognize that the included file is math.h and create a new math.h. Where would this file reside?

Clang's implementation-specific headers live in `clang/lib/Headers` and we don't currently have one for `math.h` so you'd have to add it.

>> - We do checks on every typedef declaration.  There's a builtin-identifier trick that we do with library functions that we should be able to generalize to typedefs, so you wouldn't need to actually do string comparisons, you'd just check whether the `IdentifierInfo*` was storing a special ID.  We'd set that up in `initializeBuiltins` at the start of the translation unit, so the overhead would just be that we'd create two extra `IdentifierInfo*`s in every TU.
>>
>> The builtin ID logic is currently specific to builtin *functions*, and I don't think we'd want to hack typedef names into that.  But the same storage in `IdentifierInfo*` is also used for identifying the ObjC context-sensitive keywords, by just offsetting the builtin IDs by the NUM_OBJC_KEYWORD.  You should be able to generalize that by also introducing a concept of a builtin *type* name, so that e.g. IDs in [0,NUM_OBJC_KEYWORDS) are ObjCKeywordKinds, IDs in [NUM_OBJC_KEYWORDS, NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES) are BuiltinTypeKinds (when you subtract NUM_OBJC_KEYWORDS), and IDs in [NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES,∞) are builtin function IDs (when you subtract NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES).
>
> Need to look at this more closely to understand what you are suggesting.

Basically, he's saying that instead of doing a string comparison against the name of the typedef, we can ask the typedef for its `IdentifierInfo *` for the name and do a pointer comparison against an `IdentiferInfo *` that's cached.

> @rjmccall do you have a preference for any one of these solutions? @aaron.ballman ?
> Thanks.

My slight preference is to modify math.h, but I'd also be okay with doing the pointer comparison trick. I think there are less includes of math.h than there are typedefs in a TU, so adding a tiny bit of compile time overhead when including math.h seems to be a better tradeoff.


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

https://reviews.llvm.org/D146148