zahiraam added a comment. 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. 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
