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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D146148#4224094 <https://reviews.llvm.org/D146148#4224094>, @aaron.ballman wrote:

> In D146148#4222306 <https://reviews.llvm.org/D146148#4222306>, @zahiraam wrote:
>
>> In D146148#4221651 <https://reviews.llvm.org/D146148#4221651>, @rjmccall wrote:
>>
>>> 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.

Right.  It's not a great solution for standard stuff like this.

>>> - 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.

Right.  To be clear, this is a general thing that we already do to a bunch of other headers.  It doesn't require any special handling, the compiler's include directory is just the first entry on the search list for system headers.

>>> - 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.

That's a trick we could do, but I'm actually suggesting a step better than that.  The way we handle builtin functions is that we have bits in the `IdentifierInfo` structure saying that the identifier is a builtin name.  Those bits are generally ignored except by a few places in Sema that check for them during lookup.  We eagerly create those identifiers and set those bits at the start of the TU.  We could do the same trick for these names, so that when we're declaring a typedef at global scope we just check whether the name is special and trigger some special processing only if so.  It would add a very, very small overhead to processing every typedef declaration, comparable to the overhead we add to processing every function declaration in order to support the declaration of "builtin" library functions.

>> @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.

Yes, adding a math.h to the compiler header seems like the cleanest thing if it doesn't cause integration headaches.  The redeclaration thing would be a problem if there are math.h's that don't declare these typedefs, for example (if there isn't a way to check for that).


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

https://reviews.llvm.org/D146148