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