[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#4227088 <https://reviews.llvm.org/D146148#4227088>, @zahiraam wrote:

> In D146148#4224862 <https://reviews.llvm.org/D146148#4224862>, @rjmccall wrote:
>
>> 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).
>
> @rjmccall, @aaron.ballman and I looked over various math header files to see when float_t/double_t started. It looks like it started with C99 but is allowed with C89 with MS for example.
>
> In glibc the typedefs are guarded with an #ifdef __USE_ISOC99__.
>
> MS:
>
>   #if defined _M_IX86 && _M_IX86_FP < 2 && !defined _M_FP_FAST
>      typedef long double float_t;
>      typedef long double double_t;
>   #else
>      typedef float  float_t;
>      typedef double double_t;
>   #endif
>
> It's not guarded with the version of the standard. So, including math.h even with std=c89 would have a float_t/double_t.
>
> MacOS, the definition of float_t is not guarded either. We would have the same issue than with MS.
>
>   /*  Define float_t and double_t per C standard, ISO/IEC 9899:2011 7.12 2,       
>       taking advantage of GCC's __FLT_EVAL_METHOD__ (which a compiler may         
>       define anytime and GCC does) that shadows FLT_EVAL_METHOD (which a          
>       compiler must define only in float.h).                                    *\
>   /
>   #if __FLT_EVAL_METHOD__ == 0
>       typedef float float_t;
>       typedef double double_t;
>   #elif __FLT_EVAL_METHOD__ == 1
>       typedef double float_t;
>       typedef double double_t;
>   #elif __FLT_EVAL_METHOD__ == 2 || __FLT_EVAL_METHOD__ == -1
>
> It looks like we have an array of choices that might make the header method a bit uncontrollable.
> So, if you all agree I will start looking at the identifier solution that you are proposing above. 
> Thanks.

I think the identifier solution makes the most sense. The situation I'm mostly worried about is where the system math.h does not expose `float_t` but our math.h would then add it -- I don't think we've got the introspection capabilities to be able to determine if the typedef is or isn't present from within our math.h header file, and I don't think we want a pile of libc-specific macros guarding the typedefs.


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

https://reviews.llvm.org/D146148