[clang] Add a namespace for interesting identifiers.

[aaron.ballman]

These changes will also need a release note at some point.

clang/include/clang/Basic/DiagnosticParseKinds.td

662–665 ↗

(On Diff #505520)

I think we should add some documentation as to why this error happens. Naively, users expect the pragma to override the command line (the command line is the "default" and the pragma overrides that in the cases the default is wrong), so I'd imagine a user getting this error would ask, "But why?"

clang/lib/Parse/ParseStmt.cpp

1122–1123 ↗

(On Diff #505520)

Does something assert that we never try to call this with FEM_Indeterminable?

1198–1210 ↗

(On Diff #505520)

This is not correct -- it's only going to catch the case where the user literally writes float_t as the first token in a statement, so it's going to miss const float_t t and typedef float_t frobble; frobble t; etc. Also, this seems like it will impact compile time overhead because that's going to be performing a lot of useless string comparisons.

This cannot be handled at the level of the parser, it needs to be handled within Sema. I think you'll want it to live somewhere around GetFullTypeForDeclarator() in SemaType.cpp.

There are other situations that should be tested:

char buffer[sizeof(float_t)]; // This seems like something we'd want to prevent?
typedef float_t foo; // This seems like something that's harmless to accept?
using quux = float_t; // Same here as above
foo bar; // This, however, is a problem

extern float_t blah(); // This also seems bad

_Generic(1, float_t : 0); // Maybe a bad thing?

auto lam = [](float_t f) { return f; }; // Bad?

float f = (float_t)12; // What about this? Note, the float_t is only used in the cast...

clang/test/CodeGen/abi-check-1.c

1–2 ↗

(On Diff #505520)

Why are these codegen tests when the diagnostic (should be) in Sema?

[zahiraam]

clang/include/clang/Basic/DiagnosticParseKinds.td

662–665 ↗

(On Diff #505520)

Where do you suggest I add the documentation?

[aaron.ballman]

Where do you suggest I add the documentation?

Somewhere around https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags where we document clang fp eval_method.

[rjmccall]

The user isn't modifying the float_t type definition, they're using it. I think the diagnostic should say something like cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit.

Is there any way we can infer an attribute for these typedefs when they're declared, then diagnose it in DiagnoseUseOfDecl? Some sort of "available_only_in_default_eval_method" attribute?

[zahiraam]

In D146148#4213475, @rjmccall wrote:

The user isn't modifying the float_t type definition, they're using it. I think the diagnostic should say something like cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit.

Is there any way we can infer an attribute for these typedefs when they're declared, then diagnose it in DiagnoseUseOfDecl? Some sort of "available_only_in_default_eval_method" attribute?

@rjmccall Thanks for the review.
I might be able to call the DiagnoseUseOfDecl here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L1609 but I don't seem to have access to the NameDecl (first argument of the function).
Would the attribute be on the Decl?
Thanks.

[rjmccall]

In D146148#4217382, @zahiraam wrote:

In D146148#4213475, @rjmccall wrote:

The user isn't modifying the float_t type definition, they're using it. I think the diagnostic should say something like cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit.

Is there any way we can infer an attribute for these typedefs when they're declared, then diagnose it in DiagnoseUseOfDecl? Some sort of "available_only_in_default_eval_method" attribute?

@rjmccall Thanks for the review.
I might be able to call the DiagnoseUseOfDecl here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L1609 but I don't seem to have access to the NameDecl (first argument of the function).

We already call DiagnoseUseOfDecl whenever you use a declaration. You wouldn't need to inject any extra code into the core type-lookup operation.

Would the attribute be on the Decl?

I'm suggesting that you could add a check in DiagnoseUseOfDecl for some new attribute that you would synthesize when we build a typedef with this name. (We do something similar with library declarations of builtins.) Or, better yet, we could just modify the header to explicitly use the attribute, or maybe add the attribute in a redeclaration in our own math.h; I don't know if those are feasible options.

[rjmccall]

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11560

[zahiraam]

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

[aaron.ballman]

In D146148#4220433, @zahiraam wrote:

In 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?

[zahiraam]

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

[rjmccall]

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

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.
• We add a math.h compiler header that #include_nexts 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)).
• 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).

[zahiraam]

In D146148#4221651, @rjmccall wrote:

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

In 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"?

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_nexts 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));

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

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

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

[aaron.ballman]

In D146148#4222306, @zahiraam wrote:

In D146148#4221651, @rjmccall wrote:

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

[rjmccall]

In D146148#4224094, @aaron.ballman wrote:

In D146148#4222306, @zahiraam wrote:

In 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_nexts 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).

[zahiraam]

In D146148#4224862, @rjmccall wrote:

In D146148#4224094, @aaron.ballman wrote:

In D146148#4222306, @zahiraam wrote:

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

[aaron.ballman]

In D146148#4227088, @zahiraam wrote:

In D146148#4224862, @rjmccall wrote:

In D146148#4224094, @aaron.ballman wrote:

In D146148#4222306, @zahiraam wrote:

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

[rjmccall]

Yeah, that was the sort of thing I was worried about. The identifier thing is also a reasonable approach, and it'll be nice infrastructure for other, similar things.

The ranges of special identifier we already call out on IdentifierInfo* are for builtin functions (where we need to synthesize a declaration) and ObjC context-sensitive tokens (which are directly important to the parser). I think it makes sense for these to be a third category, but please don't make it as narrow as BuiltinTypedefKind or something like that; call it something generic like InterestingIdentifierKind, and make sure the code in ActOnTypedefNameDecl checks for the float_t and double_t kinds specifically before adding the attribute, so that people using this mechanism for other purposes don't inadvertently trigger your code. We have a number of other places in the compiler that would probably benefit from using this over doing IdentifierInfo* comparisons.

[zahiraam]

(Sorry for taking so long to get to it!) Thanks.

[rjmccall]

clang/docs/LanguageExtensions.rst

4602

Suggestion:

``math.h`` defines the typedefs ``float_t`` and ``double_t`` based on the active
evaluation method at the point where the header is included, not where the
typedefs are used.  Because of this, it is unwise to combine these typedefs with
``#pragma clang fp eval_method``.  To catch obvious bugs, Clang will emit an
error for any references to these typedefs within the scope of this pragma;
however, this is not a fool-proof protection, and programmers must take care.

clang/include/clang/Basic/IdentifierTable.h

111

The suggestion was to use a range of values within ObjCOrBuiltinID rather than adding
storage to all identifiers.

[zahiraam]

In D146148#4221651, @rjmccall wrote:

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

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.
• We add a math.h compiler header that #include_nexts 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)).
• 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).

Reading this more carefully... Does that mean that we initialize the float_t, double_t in initializeBuiltins even if they are not used in the source code?
Also not sure how to define the NUM_BUILTIN_TYPES since I don't need to add it to TokenKinds.h? I was proposing to do something like this:
enum InterestingIdentifierKind {
#define Interesting_Identifier(X) X,
#include "clang/Basic/TokenKinds.def"

NUM_INTERESTING_IDENTIFIERS

};
But I guess I don't need since we don't want to add additional storage. Do I understand things correctly?

[rjmccall]

In D146148#4330611, @zahiraam wrote:

In D146148#4221651, @rjmccall wrote:

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

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.
• We add a math.h compiler header that #include_nexts 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)).
• 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).

Reading this more carefully... Does that mean that we initialize the float_t, double_t in initializeBuiltins even if they are not used in the source code?

Yes. If we decide this is an overhead worth eliminating, we'll find a way to do it lazily to the builtins, and then we'll be able to take advantage of it here, too. The builtins are a much larger contributor to overhead.

Also not sure how to define the NUM_BUILTIN_TYPES since I don't need to add it to TokenKinds.h? I was proposing to do something like this:
enum InterestingIdentifierKind {
#define Interesting_Identifier(X) X,
#include "clang/Basic/TokenKinds.def"

NUM_INTERESTING_IDENTIFIERS

};
But I guess I don't need since we don't want to add additional storage. Do I understand things correctly?

We should have an enum like this, yes. And this is what we do in IdentifierTable.h for all the other kinds. Alternatively, you can just hard-code the number and then static_assert that it's correct in some .cpp file.

FWIW, I think I like NUM_INTERESTING_IDENTIFIERS as a name rather than NUM_BUILTIN_TYPES.

[zahiraam]

In D146148#4330971, @rjmccall wrote:

In D146148#4330611, @zahiraam wrote:

In D146148#4221651, @rjmccall wrote:

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

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.
• We add a math.h compiler header that #include_nexts 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)).
• 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).

Reading this more carefully... Does that mean that we initialize the float_t, double_t in initializeBuiltins even if they are not used in the source code?

Yes. If we decide this is an overhead worth eliminating, we'll find a way to do it lazily to the builtins, and then we'll be able to take advantage of it here, too. The builtins are a much larger contributor to overhead.

Also not sure how to define the NUM_BUILTIN_TYPES since I don't need to add it to TokenKinds.h? I was proposing to do something like this:
enum InterestingIdentifierKind {
#define Interesting_Identifier(X) X,
#include "clang/Basic/TokenKinds.def"

NUM_INTERESTING_IDENTIFIERS

};
But I guess I don't need since we don't want to add additional storage. Do I understand things correctly?

We should have an enum like this, yes. And this is what we do in IdentifierTable.h for all the other kinds. Alternatively, you can just hard-code the number and then static_assert that it's correct in some .cpp file.

FWIW, I think I like NUM_INTERESTING_IDENTIFIERS as a name rather than NUM_BUILTIN_TYPES.

Okay, so I did add that in TokenKinds.h. Isn't that the right place for it? The same way it's done for the other builtins? And in TokenKinds.def I added the lines for the interesting identifiers?

In D146148#4330971, @rjmccall wrote:

In D146148#4330611, @zahiraam wrote:

In D146148#4221651, @rjmccall wrote:

In D146148#4220591, @zahiraam wrote:

In D146148#4220495, @aaron.ballman wrote:

In D146148#4220433, @zahiraam wrote:

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

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.
• We add a math.h compiler header that #include_nexts 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)).
• 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).

Reading this more carefully... Does that mean that we initialize the float_t, double_t in initializeBuiltins even if they are not used in the source code?

Yes. If we decide this is an overhead worth eliminating, we'll find a way to do it lazily to the builtins, and then we'll be able to take advantage of it here, too. The builtins are a much larger contributor to overhead.

Also not sure how to define the NUM_BUILTIN_TYPES since I don't need to add it to TokenKinds.h? I was proposing to do something like this:
enum InterestingIdentifierKind {
#define Interesting_Identifier(X) X,
#include "clang/Basic/TokenKinds.def"

NUM_INTERESTING_IDENTIFIERS

};
But I guess I don't need since we don't want to add additional storage. Do I understand things correctly?

We should have an enum like this, yes. And this is what we do in IdentifierTable.h for all the other kinds. Alternatively, you can just hard-code the number and then static_assert that it's correct in some .cpp file.

FWIW, I think I like NUM_INTERESTING_IDENTIFIERS as a name rather than NUM_BUILTIN_TYPES.

Okay but should this be added to TokenKinds.def or Builtin.defs? the patch is adding it to TokenKinds.

[rjmccall]

Okay, so I did add that in TokenKinds.h. Isn't that the right place for it? The same way it's done for the other builtins? And in TokenKinds.def I added the lines for the interesting identifiers?

What you're doing in TokenKinds.{def,h} seems fine. What I'm objecting to is adding another field to IdentifierInfo to store it instead of fitting it into ObjCOrBuiltinID. Currently an given identifier is either normal, an ObjC keyword, or a builtin ID, and it determines this by checking what range of values ObjCOrBuiltinID fits into. You can use the same idea to allow an identifier to be either normal, an ObjC keyword, a builtin ID, or an interesting.

clang/include/clang/Basic/TokenKinds.def

89

Please follow the existing pattern by spelling this INTERESTING_IDENTIFIER.

[zahiraam]

@rjmccall Made a bit of progress on this, but still not quite where it should be! Would appreciate your feedback/guidance to see that's moving in the right direction. Not sure about lazily creating the interesting identifiers? There are still some LIT tests failing but will care for those when I get your feedback. Thanks.

[rjmccall]

clang/include/clang/Basic/IdentifierTable.h

347

This is closer to the right approach, thanks. I think the best way to do this is to put the ObjC keywords first, then the interesting identifiers, then the builtins. That means the builtin ID code above should check for / add / subtract tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS; please add a FirstBuiltinID constant for that in this class so that we can more easily rework things in the future (e.g. if we decide to stuff more things into this field). And then the code for InterestingIdentifierID should add/subtract tok::NUM_OBJC_KEYWORDS; again, please add a FirstInterestingIdentifier constant for that.

Please make these two functions use tok::InterestingIdentifierKind. For getInterestingIdentifierID(), you'll have to decide how you want to represent that something isn't an interesting identifier. ObjCKeywordKind has a special enumerator (not_keyword) at value 0, so you could do the same thing here, but I think it might be better to use llvm::Optional.

[zahiraam]

clang/include/clang/Basic/IdentifierTable.h

347

This is closer to the right approach, thanks. I think the best way to do this is to put the ObjC keywords first, then the interesting identifiers, then the builtins. That means the builtin ID code above should check for / add / subtract tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS; please add a FirstBuiltinID constant for that in this class so that we can more easily rework things in the future (e.g. if we decide to stuff more things into this field). And then the code for InterestingIdentifierID should add/subtract tok::NUM_OBJC_KEYWORDS; again, please add a FirstInterestingIdentifier constant for that.

I have made the changes in this new version of the patch.

Please make these two functions use tok::InterestingIdentifierKind. For getInterestingIdentifierID(), you'll have to decide how you want to represent that something isn't an interesting identifier. ObjCKeywordKind has a special enumerator (not_keyword) at value 0, so you could do the same thing here, but I think it might be better to use llvm::Optional.

However, I am stumped for this. If we want to use tok::InterestingIdentifierKind, then the addition of the interesting Identifiers need to be made within IdentifierTable::Addkeywords (by adding a new macro). If we do it the way it's done in this patch in Builtin::Context::initializeBuiltins then tok::InterestingIdentifierKind can't be used. Unless I am missing something?

Also, I left the tok::not_interesting only because not really sure how to use the llvm::Optional.

[zahiraam]

ping.

[zahiraam]

@rjmccall any chance we get your feedback on this please? Thanks.

[rjmccall]

clang/include/clang/Basic/Builtins.h

65 ↗

(On Diff #524882)

You shouldn't muddle this into Builtin::ID.

clang/include/clang/Basic/IdentifierTable.h

79

// The "layout" of ObjCOrBuiltinID is:
//   - The first value (0) represents "not a special identifier".
//   - The next (NUM_OBJC_KEYWORDS - 1) values represent ObjCKeywordKinds (not
//      including objc_not_keyword).
//   - The next (NUM_INTERESTING_IDENTIFIERS - 1) values represent InterestingIdentifierKinds
//      (not including not_interesting).
//   - The rest of the values represent builtin IDs (not including not_builtin).
static constexpr int FirstObjCKeywordID = 1;
static constexpr int LastObjCKeywordID = FirstObjCKeywordID + tok::NUM_OBJC_KEYWORDS - 2;
static constexpr int FirstInterestingIdentifierID = LastObjCKeywordID + 1;
static constexpr int LastInterestingIdentifierID = LastObjCKeywordID + tok::NUM_INTERESTING_IDENTIFIERS - 2;
static constexpr int FirstBuiltinID = LastInterestingIdentifierID + 1;

308–310

static_assert(FirstObjCKeywordID == 1, "hard-coding this assumption to simplify code");
if (ObjCOrBuiltinID <= LastObjCKeywordID)
  return tok::ObjCKeywordKind(ObjCOrBuiltinID);
else
  return tok::objc_not_keyword;

321–322

if (ObjCOrBuiltinID >= FirstBuiltinID)
  return 1 + (ObjCOrBuiltinID - FirstBuiltinID);
else
  return 0;

327–343

assert(ID != NotBuiltin);
ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
assert(getBuiltinID() == ID && "ID too large for field!");

336

tok::InterestingIdentifierKind getInterestingIdentifierID() const {
  if (ObjCOrBuiltinID >= FirstInterestingIdentifierID && ObjCOrBuiltinID <= LastInterestingIdentifierID)
    return tok::InterestingIdentifierKind(1 + (ObjCOrBuiltinID - FirstInterestingIdentifierID));
  else
    return tok::not_interesting;
}

[zahiraam]

clang/lib/Basic/Builtins.cpp

33

From your comment in Builtin.h: "You shouldn't muddle this into Builtin::ID."
This means this shouldn't happen here. It should be done in IdentiferTable::AddKeywords as I had done previously?

[rjmccall]

clang/lib/Basic/Builtins.cpp

33

You shouldn't be adding entries for the interesting keywords to the builtins table, no. This table is indexed by Builtin::ID.

I agree that AddKeywords seems like the right place to set up the interesting identifiers rather than initializeBuiltins.

137

You shouldn't use FirstBuiltinID and FirstInterestingIdentifierID as the starting points here.

[zahiraam]

Thanks.

[rjmccall]

clang/include/clang/Basic/IdentifierTable.h

85

The code below does not represent NotBuiltin (that's what adding and subtracting 1 does).

89

I see that I had a bug in my suggestion: I had meant to write LastInterestingIdentifierID = FirstInterestingIdentifierID + tok::NUM_INTERESTING_IDENTIFIERS - 2; but I left it in terms of LastObjCKeywordID instead, making the ranges off by 1. Your math fixes that; sorry about that. I do think it would be clearer if each of these chained off the last one, the way I meant to have it, though. So with your ranges (which leave space to explicitly represent not_interesting), that would look like LastInterestingIdentifierID = FirstInterestingIdentifierID + tok::NUM_INTERESTING_IDENTIFIERS - 1;.

I'm not going to push you to not represent not_interesting, since you seem to have deliberately changed things back that way, and I don't think it matters that much. Although maybe you did that just because it didn't work in the code I gave you? It would be more consistent with the other enums to not explicitly represent not_interesting.

327–343

initializeBuiltins does call setBuiltinID(NotBuiltin) in order to reset any identifiers for which we have a -fno-builtin=X argument, so you need to handle that somehow, because otherwise this is going to underflow and set up the identifier with the last InterestingIdentifierKind. There are two options:

• You could just make initializeBuiltins call some sort of clearBuiltinID method that resets ObjCOrBuiltinID to 0. That has the advantage of making the "lifecycle" of this field much more straightforward. For example, we probably ought to be asserting in these methods that we're not overwriting existing data; clearBuiltinID would know that it was okay to clear data, but only from a builtin.
• You could also just check for NotBuiltin and set ObjCBuiltinID = 0 when you see it.

[zahiraam]

clang/include/clang/Basic/IdentifierTable.h

89

Sorry @rjmccall, I didn't mean to go against your proposal. It's a misunderstanding of my part. I was going through the debugger with an objc test case and was under the impression that not_keyword was added. Will make the change to not represent not_interesting.

[zahiraam]

clang/include/clang/Basic/IdentifierTable.h

327–343

initializeBuiltins does call setBuiltinID(NotBuiltin) in order to reset any identifiers for which we have a -fno-builtin=X argument, so you need to handle that somehow, because otherwise this is going to underflow and set up the identifier with the last InterestingIdentifierKind. There are two options:

• You could just make initializeBuiltins call some sort of clearBuiltinID method that resets ObjCOrBuiltinID to 0. That has the advantage of making the "lifecycle" of this field much more straightforward. For example, we probably ought to be asserting in these methods that we're not overwriting existing data; clearBuiltinID would know that it was okay to clear data, but only from a builtin.

Would that be an additional step in initializeBuiltins or part of step #4? I understants intuitively what the issue would be here if we didn't do that, but I can't find a test case that goes through that step. Would be clearer to me.

• You could also just check for NotBuiltin and set ObjCBuiltinID = 0 when you see it.

I don't think this can be done here. builtin::NotBuiltin is not known here in IdentierTable.h (get a lots of build errors). So, this needs to be done in initializerBultins method.

[rjmccall]

clang/include/clang/Basic/IdentifierTable.h

327–343

Would that be an additional step in initializeBuiltins or part of step #4? I understants intuitively what the issue would be here if we didn't do that, but I can't find a test case that goes through that step. Would be clearer to me.

Yeah, it'd be step 4: instead of calling Table.get(Name).setBuiltinID(Builtin::NotBuiltin);, it would call Table.get(Name).clearBuiltinID().

...although while you're editing that code, please change Table.get(Name) to NameIt->second. We've already looked up the IdentifierInfo and don't need to do it again.

I don't think this can be done here. builtin::NotBuiltin is not known here in IdentierTable.h (get a lots of build errors). So, this needs to be done in initializerBultins method.

Okay, that works for me. Please at least assert that the argument to setBuiltinID is not this value. It's acceptable to assume that the value is 0 — we're doing that already with the -1 / +1.

[zahiraam]

clang/include/clang/Basic/IdentifierTable.h

328

Is this the assert you are looking for?

[rjmccall]

clang/include/clang/Basic/IdentifierTable.h

328

No, I want you to assert that ID (the argument) is not 0, because if it is you'll end up setting ObjCOrBuiltinID to FirstBuiltinID - 1, i.e. LastInterestingIdentifierID.

341

Similarly, this assertion needs to be != tok::not_interesting (i.e. != 0).

[rjmccall]

clang/lib/Sema/SemaDecl.cpp

6784

Please switch over the interesting identifiers here; we don't want to assume this feature is only used for these two names.

In fact, should we go ahead and immediately apply it to the four identifiers above this? That would be nice, because then we could actually do this in two patches: one patch that does the refactor to track interesting identifiers but doesn't cause any functionality changes and a second, very small patch that just introduces the new special treatment for float_t and double_t.

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

6784

Please switch over the interesting identifiers here; we don't want to assume this feature is only used for these two names.

In fact, should we go ahead and immediately apply it to the four identifiers above this? That would be nice, because then we could actually do this in two patches: one patch that does the refactor to track interesting identifiers but doesn't cause any functionality changes and a second, very small patch that just introduces the new special treatment for float_t and double_t.

Are you saying that "FILE", "jmp_buf"," sigjmp_buf" and "ucontext_t" are also interesting identifiers? If yes, they should be added to the list of interesting identifiers in TokenKinds.def?

[rjmccall]

clang/lib/Sema/SemaDecl.cpp

6784

Right. The basic idea of interesting identifiers is to replace these sorts of identifier comparisons in performance-critical code. So your first patch would *only* add those four identifiers as interesting identifiers, handling them here by registering the typedef with the ASTContext like the code is already doing. Then you'd make a follow-up patch that adds float_t and double_t and handles them here by implicitly adding your new attribute.

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

6784

Right. The basic idea of interesting identifiers is to replace these sorts of identifier comparisons in performance-critical code. So your first patch would *only* add those four identifiers as interesting identifiers, handling them here by registering the typedef with the ASTContext like the code is already doing. Then you'd make a follow-up patch that adds float_t and double_t and handles them here by implicitly adding your new attribute.

I think that does it?

[rjmccall]

Yes, this is very close, thank you.

clang/include/clang/Basic/TokenKinds.def

805

I think it would be cleaner if you added these two as interesting identifiers in your follow-up patch.

clang/lib/Sema/SemaLookup.cpp

950

The two changes in this file shouldn't be necessary, since interesting identifiers should be disjoint from builtins.

[rjmccall]

Thanks, LGTM!

clang/docs/LanguageExtensions.rst

.. code-block:: c++
 
   for(...) {
     // The compiler will use long double as the floating-point evaluation
     // method.
     #pragma clang fp eval_method(extended)
     a = b[i] * c[i] + e;
   }
 
+Note: ``math.h`` defines the typedefs ``float_t`` and ``double_t`` based on the active
+evaluation method at the point where the header is included, not where the
+typedefs are used.  Because of this, it is unwise to combine these typedefs with
+``#pragma clang fp eval_method``.  To catch obvious bugs, Clang will emit an
+error for any references to these typedefs within the scope of this pragma;
+however, this is not a fool-proof protection, and programmers must take care.
+
 The ``#pragma float_control`` pragma allows precise floating-point
 semantics and floating-point exception behavior to be specified
 for a section of the source code. This pragma can only appear at file or
 namespace scope, within a language linkage specification or at the start of a
 compound statement (excluding comments). When used within a compound statement,
 the pragma is active within the scope of the compound statement.  This pragma
 is modeled after a Microsoft pragma with the same spelling and syntax.  For
 pragmas specified at file or namespace scope, or within a language linkage

clang/include/clang/Basic/Attr.td

def WebAssemblyFuncref : TypeAttr, TargetSpecificAttr<TargetWebAssembly> {
   let Subjects = SubjectList<[FunctionPointer], ErrorDiag>;
 }
 
 def ReadOnlyPlacement : InheritableAttr {
   let Spellings = [Clang<"enforce_read_only_placement">];
   let Subjects = SubjectList<[Record]>;
   let Documentation = [ReadOnlyPlacementDocs];
 }
+
+
+def AvailableOnlyInDefaultEvalMethod : InheritableAttr {
+  let Spellings = [Clang<"available_only_in_default_eval_method">];
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+  let Documentation = [Undocumented];
+}

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_objc_type_args_non_parameterized_class : Error<
   "type arguments cannot be applied to non-parameterized class %0">;
 
 def err_objc_type_args_specialized_class : Error<
   "type arguments cannot be applied to already-specialized class type %0">;
 
 def err_objc_type_args_wrong_arity : Error<
   "too %select{many|few}0 type arguments for class %1 (have %2, expected %3)">;
 }
 
+def err_type_available_only_in_default_eval_method : Error<
+  "cannot use type '%0' within '#pragma clang fp eval_method'; type is set "
+  "according to the default eval method for the translation unit">;
+
 def err_objc_type_arg_not_id_compatible : Error<
   "type argument %0 is neither an Objective-C object nor a block type">;
 
 def err_objc_type_arg_does_not_match_bound : Error<
   "type argument %0 does not satisfy the bound (%1) of type parameter %2">;
 
 def warn_objc_redundant_qualified_class_type : Warning<
   "parameterized class %0 already conforms to the protocols listed; did you "

clang/include/clang/Basic/IdentifierTable.h

 
 /// IdentifierInfo and other related classes are aligned to
 /// 8 bytes so that DeclarationName can use the lower 3 bits
 /// of a pointer to one of these classes.
 enum { IdentifierInfoAlignment = 8 };
 
 static constexpr int ObjCOrBuiltinIDBits = 16;
 
+/// The "layout" of ObjCOrBuiltinID is:
+///  - The first value (0) represents "not a special identifier".
+///  - The next (NUM_OBJC_KEYWORDS - 1) values represent ObjCKeywordKinds (not
+///    including objc_not_keyword).
+///  - The next (NUM_INTERESTING_IDENTIFIERS - 1) values represent
+///    InterestingIdentifierKinds (not including not_interesting).
+///  - The rest of the values represent builtin IDs (not including NotBuiltin).
+static constexpr int FirstObjCKeywordID = 1;
+static constexpr int LastObjCKeywordID =
+    FirstObjCKeywordID + tok::NUM_OBJC_KEYWORDS - 2;
+static constexpr int FirstInterestingIdentifierID = LastObjCKeywordID + 1;
+static constexpr int LastInterestingIdentifierID =
+    FirstInterestingIdentifierID + tok::NUM_INTERESTING_IDENTIFIERS - 2;
+static constexpr int FirstBuiltinID = LastInterestingIdentifierID + 1;
+
 /// One of these records is kept for each identifier that
 /// is lexed.  This contains information about whether the token was \#define'd,
 /// is a language keyword, or if it is a front-end token of some sort (e.g. a
 /// variable or function name).  The preprocessor keeps this information in a
 /// set, and all tok::identifier tokens have a pointer to one of these.
 /// It is aligned to 8 bytes because DeclarationName needs the lower 3 bits.
 class alignas(IdentifierInfoAlignment) IdentifierInfo {
   friend class IdentifierTable;
 
   // Front-end token ID or tok::identifier.
   unsigned TokenID : 9;
 
   // ObjC keyword ('protocol' in '@protocol') or builtin (__builtin_inf).
   // First NUM_OBJC_KEYWORDS values are for Objective-C,
   // the remaining values are for builtins.
   unsigned ObjCOrBuiltinID : ObjCOrBuiltinIDBits;
 
   // True if there is a #define for this.
   unsigned HasMacro : 1;
 
   // True if there was a #define for this.
   unsigned HadMacro : 1;
 
   // True if the identifier is a language extension.
   unsigned IsExtension : 1;
 
public:
   ///
   /// For example, "define" will return tok::pp_define.
   tok::PPKeywordKind getPPKeywordID() const;
 
   /// Return the Objective-C keyword ID for the this identifier.
   ///
   /// For example, 'class' will return tok::objc_class if ObjC is enabled.
   tok::ObjCKeywordKind getObjCKeywordID() const {
-    if (ObjCOrBuiltinID < tok::NUM_OBJC_KEYWORDS)
+    static_assert(FirstObjCKeywordID == 1,
+                  "hard-coding this assumption to simplify code");
+    if (ObjCOrBuiltinID <= LastObjCKeywordID)
       return tok::ObjCKeywordKind(ObjCOrBuiltinID);
     else
       return tok::objc_not_keyword;
   }
   void setObjCKeywordID(tok::ObjCKeywordKind ID) { ObjCOrBuiltinID = ID; }
 
   /// Return a value indicating whether this is a builtin function.
   ///
   /// 0 is not-built-in. 1+ are specific builtin functions.
   unsigned getBuiltinID() const {
-    if (ObjCOrBuiltinID >= tok::NUM_OBJC_KEYWORDS)
-      return ObjCOrBuiltinID - tok::NUM_OBJC_KEYWORDS;
+    if (ObjCOrBuiltinID >= FirstBuiltinID)
+      return 1 + (ObjCOrBuiltinID - FirstBuiltinID);
     else
       return 0;
   }
   void setBuiltinID(unsigned ID) {
-    ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS;
-    assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID
-           && "ID too large for field!");
+    ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
+    assert(getBuiltinID() == ID && "ID too large for field!");
+  }
+  void clearBuiltinID() { ObjCOrBuiltinID = 0; }
+
+  tok::InterestingIdentifierKind getInterestingIdentifierID() const {
+    if (ObjCOrBuiltinID >= FirstInterestingIdentifierID &&
+        ObjCOrBuiltinID <= LastInterestingIdentifierID)
+      return tok::InterestingIdentifierKind(
+          1 + (ObjCOrBuiltinID - FirstInterestingIdentifierID));
+    else
+      return tok::not_interesting;
+  }
+  void setInterestingIdentifierID(unsigned ID) {
+    assert(ID != FirstBuiltinID);
+    ObjCOrBuiltinID = FirstInterestingIdentifierID + (ID - 1);
+    assert(getInterestingIdentifierID() == ID && "ID too large for field!");
   }
 
   unsigned getObjCOrBuiltinID() const { return ObjCOrBuiltinID; }
   void setObjCOrBuiltinID(unsigned ID) { ObjCOrBuiltinID = ID; }
 
   /// get/setExtension - Initialize information about whether or not this
   /// language token is an extension.  This controls extension warnings, and is
   /// only valid if a custom token ID is set.
   bool isExtensionToken() const { return IsExtension; }
   void setIsExtensionToken(bool Val) {
     IsExtension = Val;
     if (Val)

clang/include/clang/Basic/TokenKinds.h

 /// Provides a namespace for Objective-C keywords which start with
 /// an '@'.
 enum ObjCKeywordKind {
 #define OBJC_AT_KEYWORD(X) objc_##X,
 #include "clang/Basic/TokenKinds.def"
   NUM_OBJC_KEYWORDS
 };
 
+/// Provides a namespace for interesting identifers such as float_t and
+/// double_t.
+enum InterestingIdentifierKind {
+#define INTERESTING_IDENTIFIER(X) X,
+#include "clang/Basic/TokenKinds.def"
+  NUM_INTERESTING_IDENTIFIERS
+};
+
 /// Defines the possible values of an on-off-switch (C99 6.10.6p2).
 enum OnOffSwitch {
   OOS_ON, OOS_OFF, OOS_DEFAULT
 };
 
 /// Determines the name of a token as used within the front end.
 ///
 /// The name of a token will be an internal name (such as "l_square")

clang/include/clang/Basic/TokenKinds.def

 #define TESTING_KEYWORD(X, L) KEYWORD(X, L)
 #endif
 #ifndef ANNOTATION
 #define ANNOTATION(X) TOK(annot_ ## X)
 #endif
 #ifndef PRAGMA_ANNOTATION
 #define PRAGMA_ANNOTATION(X) ANNOTATION(X)
 #endif
+#ifndef INTERESTING_IDENTIFIER
+#define INTERESTING_IDENTIFIER(X)
+#endif
 
 //===----------------------------------------------------------------------===//
 // Preprocessor keywords.
 //===----------------------------------------------------------------------===//
 
 // These have meaning after a '#' at the start of a line. These define enums in
 // the tok::pp_* namespace.  Note that IdentifierInfo::getPPKeywordID must be
 // manually updated if something is added here.

 OBJC_AT_KEYWORD(package)
 OBJC_AT_KEYWORD(required)
 OBJC_AT_KEYWORD(optional)
 OBJC_AT_KEYWORD(synthesize)
 OBJC_AT_KEYWORD(dynamic)
 OBJC_AT_KEYWORD(import)
 OBJC_AT_KEYWORD(available)
 
+//===----------------------------------------------------------------------===//
+// Interesting idenitifiers.
+//===----------------------------------------------------------------------===//
+INTERESTING_IDENTIFIER(not_interesting)
+INTERESTING_IDENTIFIER(float_t)
+INTERESTING_IDENTIFIER(double_t)
+
 // TODO: What to do about context-sensitive keywords like:
 //       bycopy/byref/in/inout/oneway/out?
 
 ANNOTATION(cxxscope)     // annotation for a C++ scope spec, e.g. "::foo::bar::"
 ANNOTATION(typename)     // annotation for a C typedef name, a C++ (possibly
                          // qualified) typename, e.g. "foo::MyClass", or
                          // template-id that names a type ("std::vector<int>")
 ANNOTATION(template_id)  // annotation for a C++ template-id that names a

 #undef TYPE_TRAIT
 #undef CXX20_KEYWORD
 #undef CXX11_KEYWORD
 #undef KEYWORD
 #undef PUNCTUATOR
 #undef TOK
 #undef C99_KEYWORD
 #undef C2X_KEYWORD
+#undef INTERESTING_IDENTIFIER

clang/lib/Basic/Builtins.cpp

case ID:                                                                     \
     return NAME;
 #include "clang/Basic/BuiltinHeaders.def"
 #undef HEADER
   };
   llvm_unreachable("Unknown HeaderDesc::HeaderID enum");
 }
 
 static constexpr Builtin::Info BuiltinInfo[] = {
     {"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER,
      ALL_LANGUAGES},
 #define BUILTIN(ID, TYPE, ATTRS)                                               \
   {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, ALL_LANGUAGES},
 #define LANGBUILTIN(ID, TYPE, ATTRS, LANGS)                                    \
   {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANGS},
 #define LIBBUILTIN(ID, TYPE, ATTRS, HEADER, LANGS)                             \
   {#ID, TYPE, ATTRS, nullptr, HeaderDesc::HEADER, LANGS},
 #include "clang/Basic/Builtins.def"
void Builtin::Context::initializeBuiltins(IdentifierTable &Table,
                                           const LangOptions& LangOpts) {
   // Step #1: mark all target-independent builtins with their ID's.
   for (unsigned i = Builtin::NotBuiltin+1; i != Builtin::FirstTSBuiltin; ++i)
     if (builtinIsSupported(BuiltinInfo[i], LangOpts)) {
       Table.get(BuiltinInfo[i].Name).setBuiltinID(i);
     }
 
   // Step #2: Register target-specific builtins.
   for (unsigned i = 0, e = TSRecords.size(); i != e; ++i)
     if (builtinIsSupported(TSRecords[i], LangOpts))
       Table.get(TSRecords[i].Name).setBuiltinID(i + Builtin::FirstTSBuiltin);
 
   // Step #3: Register target-specific builtins for AuxTarget.
   for (unsigned i = 0, e = AuxTSRecords.size(); i != e; ++i)
     Table.get(AuxTSRecords[i].Name)
         .setBuiltinID(i + Builtin::FirstTSBuiltin + TSRecords.size());
 
   // Step #4: Unregister any builtins specified by -fno-builtin-foo.
   for (llvm::StringRef Name : LangOpts.NoBuiltinFuncs) {
     bool InStdNamespace = Name.consume_front("std-");
     auto NameIt = Table.find(Name);
     if (NameIt != Table.end()) {
       unsigned ID = NameIt->second->getBuiltinID();
       if (ID != Builtin::NotBuiltin && isPredefinedLibFunction(ID) &&
           isInStdNamespace(ID) == InStdNamespace) {
-        Table.get(Name).setBuiltinID(Builtin::NotBuiltin);
+        NameIt->second->clearBuiltinID();
       }
     }
   }
 }
 
 unsigned Builtin::Context::getRequiredVectorWidth(unsigned ID) const {
   const char *WidthPos = ::strchr(getRecord(ID).Attributes, 'V');
   if (!WidthPos)

clang/lib/Basic/IdentifierTable.cpp

 /// AddObjCKeyword - Register an Objective-C \@keyword like "class" "selector"
 /// or "property".
 static void AddObjCKeyword(StringRef Name,
                            tok::ObjCKeywordKind ObjCID,
                            IdentifierTable &Table) {
   Table.get(Name).setObjCKeywordID(ObjCID);
 }
 
+static void AddInterestingIdentifier(StringRef Name,
+                                     tok::InterestingIdentifierKind BTID,
+                                     IdentifierTable &Table) {
+  IdentifierInfo &Info = Table.get(Name, tok::identifier);
+  Info.setInterestingIdentifierID(BTID);
+}
+
 /// AddKeywords - Add all keywords to the symbol table.
 ///
 void IdentifierTable::AddKeywords(const LangOptions &LangOpts) {
   // Add keywords and tokens for the current language.
 #define KEYWORD(NAME, FLAGS) \
   AddKeyword(StringRef(#NAME), tok::kw_ ## NAME,  \
              FLAGS, LangOpts, *this);
 #define ALIAS(NAME, TOK, FLAGS) \
   AddKeyword(StringRef(NAME), tok::kw_ ## TOK,  \
              FLAGS, LangOpts, *this);
 #define CXX_KEYWORD_OPERATOR(NAME, ALIAS) \
   if (LangOpts.CXXOperatorNames)          \
     AddCXXOperatorKeyword(StringRef(#NAME), tok::ALIAS, *this);
 #define OBJC_AT_KEYWORD(NAME)  \
   if (LangOpts.ObjC)           \
     AddObjCKeyword(StringRef(#NAME), tok::objc_##NAME, *this);
+#define INTERESTING_IDENTIFIER(NAME)  \
+  AddInterestingIdentifier(StringRef(#NAME), tok::##NAME, *this);
+
 #define TESTING_KEYWORD(NAME, FLAGS)
 #include "clang/Basic/TokenKinds.def"
 
   if (LangOpts.ParseUnknownAnytype)
     AddKeyword("__unknown_anytype", tok::kw___unknown_anytype, KEYALL,
                LangOpts, *this);
 
   if (LangOpts.DeclSpecKeyword)

clang/lib/Sema/SemaDecl.cpp

NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
   } else {
     New = ActOnVariableDeclarator(S, D, DC, TInfo, Previous, TemplateParamLists,
                                   AddToScope);
   }
 
   if (!New)
     return nullptr;
 
+  if (New->hasAttr<AvailableOnlyInDefaultEvalMethodAttr>()) {
+    if (getLangOpts().getFPEvalMethod() !=
+            LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine &&
+        PP.getLastFPEvalPragmaLocation().isValid() &&
+        PP.getCurrentFPEvalMethod() != getLangOpts().getFPEvalMethod())
+      Diag(New->getLocation(),
+           diag::err_type_available_only_in_default_eval_method)
+          << New->getName();
+  }
   // If this has an identifier and is not a function template specialization,
   // add it to the scope stack.
   if (New->getDeclName() && AddToScope)
     PushOnScopeChains(New, S);
 
   if (isInOpenMPDeclareTargetContext())
     checkDeclIsAllowedInOpenMPTarget(nullptr, New);
 
if (!NewTD->isInvalidDecl() &&
       if (II->isStr("FILE"))
         Context.setFILEDecl(NewTD);
       else if (II->isStr("jmp_buf"))
         Context.setjmp_bufDecl(NewTD);
       else if (II->isStr("sigjmp_buf"))
         Context.setsigjmp_bufDecl(NewTD);
       else if (II->isStr("ucontext_t"))
         Context.setucontext_tDecl(NewTD);
+      if (II->getInterestingIdentifierID() != 0)
+        NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context));
     }
 
   return NewTD;
 }
 
 /// Determines whether the given declaration is an out-of-scope
 /// previous declaration.
 ///

clang/lib/Sema/SemaDeclAttr.cpp

static void handleFunctionReturnThunksAttr(Sema &S, Decl *D,
   }
   // FIXME: it would be good to better handle attribute merging rather than
   // silently replacing the existing attribute, so long as it does not break
   // the expected codegen tests.
   D->dropAttr<FunctionReturnThunksAttr>();
   D->addAttr(FunctionReturnThunksAttr::Create(S.Context, Kind, AL));
 }
 
+static void handleAvailableOnlyInDefaultEvalMethod(Sema &S, Decl *D,
+                                                   const ParsedAttr &AL) {
+  assert(isa<TypedefNameDecl>(D) && "This attribute only applies to a typedef");
+  handleSimpleAttribute<AvailableOnlyInDefaultEvalMethodAttr>(S, D, AL);
+}
+
 static void handleSYCLKernelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   // The 'sycl_kernel' attribute applies only to function templates.
   const auto *FD = cast<FunctionDecl>(D);
   const FunctionTemplateDecl *FT = FD->getDescribedFunctionTemplate();
   assert(FT && "Function template is expected");
 
   // Function template must have at least two template parameters.
   const TemplateParameterList *TL = FT->getTemplateParameters();
case ParsedAttr::AT_InternalLinkage:
     break;
   case ParsedAttr::AT_ZeroCallUsedRegs:
     handleZeroCallUsedRegsAttr(S, D, AL);
     break;
   case ParsedAttr::AT_FunctionReturnThunks:
     handleFunctionReturnThunksAttr(S, D, AL);
     break;
 
+  case ParsedAttr::AT_AvailableOnlyInDefaultEvalMethod:
+    handleAvailableOnlyInDefaultEvalMethod(S, D, AL);
+    break;
+
   // Microsoft attributes:
   case ParsedAttr::AT_LayoutVersion:
     handleLayoutVersion(S, D, AL);
     break;
   case ParsedAttr::AT_Uuid:
     handleUuidAttr(S, D, AL);
     break;
   case ParsedAttr::AT_MSInheritance:

clang/lib/Sema/SemaExpr.cpp

bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
 
   DiagnoseAvailabilityOfDecl(D, Locs, UnknownObjCClass, ObjCPropertyAccess,
                              AvoidPartialAvailabilityChecks, ClassReceiver);
 
   DiagnoseUnusedOfDecl(*this, D, Loc);
 
   diagnoseUseOfInternalDeclInInlineFunction(*this, D, Loc);
 
+  if (D->hasAttr<AvailableOnlyInDefaultEvalMethodAttr>()) {
+    if (getLangOpts().getFPEvalMethod() !=
+      LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine &&
+        PP.getLastFPEvalPragmaLocation().isValid() &&
+        PP.getCurrentFPEvalMethod() != getLangOpts().getFPEvalMethod())
+      Diag(D->getLocation(),
+           diag::err_type_available_only_in_default_eval_method)
+          << D->getName();
+  }
+
   if (auto *VD = dyn_cast<ValueDecl>(D))
     checkTypeSupport(VD->getType(), Loc, VD);
 
   if (LangOpts.SYCLIsDevice || (LangOpts.OpenMP && LangOpts.OpenMPIsDevice)) {
     if (!Context.getTargetInfo().isTLSSupported())
       if (const auto *VD = dyn_cast<VarDecl>(D))
         if (VD->getTLSKind() != VarDecl::TLS_None)
           targetDiag(*Locs.begin(), diag::err_thread_unsupported);

clang/lib/Sema/SemaLookup.cpp

void LookupResult::configure() {
 
   default:
     break;
   }
 
   // Compiler builtins are always visible, regardless of where they end
   // up being declared.
   if (IdentifierInfo *Id = NameInfo.getName().getAsIdentifierInfo()) {
+     if (!Id->getInterestingIdentifierID())
        if (unsigned BuiltinID = Id->getBuiltinID()) {
          if (!getSema().Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID))
            AllowHidden = true;
     }
   }
 }
 
 bool LookupResult::checkDebugAssumptions() const {
   // This function is never called by NDEBUG builds.
   assert(ResultKind != NotFound || Decls.size() == 0);
   assert(ResultKind != Found || Decls.size() == 1);
if (II) {
           RVIntrinsicManager = CreateRISCVIntrinsicManager(*this);
 
         if (RVIntrinsicManager->CreateIntrinsicIfFound(R, II, PP))
           return true;
       }
 
       // If this is a builtin on this (or all) targets, create the decl.
       if (unsigned BuiltinID = II->getBuiltinID()) {
+        if (unsigned ID = II->getInterestingIdentifierID()) {
+          // Don't think we should LazyCreate the interesting identifier?
+          return true;
+        }
         // In C++ and OpenCL (spec v1.2 s6.9.f), we don't have any predefined
         // library functions like 'malloc'. Instead, we'll just error.
         if ((getLangOpts().CPlusPlus || getLangOpts().OpenCL) &&
             Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID))
           return false;
 
         if (NamedDecl *D =
                 LazilyCreateBuiltin(II, BuiltinID, TUScope,

clang/test/Sema/abi-check-1.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -DNOERROR %s
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=source -DNOERROR %s
+
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=double %s
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=extended %s
+
+#ifdef NOERROR
+// expected-no-diagnostics
+typedef float float_t;
+typedef double double_t;
+#else
+typedef float float_t; //expected-error 9 {{cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit}}
+
+typedef double double_t; //expected-error 9 {{cannot use type 'double_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit}}
+#endif
+
+float foo1() {
+#pragma clang fp eval_method(source)
+  float a;
+  double b;
+  return a - b;
+}
+
+float foo2() {
+#pragma clang fp eval_method(source)
+  float_t a; 
+  double_t b; 
+  return a - b;
+}
+
+void foo3() {
+#pragma clang fp eval_method(source)
+  char buff[sizeof(float_t)];
+  char bufd[sizeof(double_t)];
+  buff[1] = bufd[2];
+}
+
+float foo4() {
+#pragma clang fp eval_method(source)
+  typedef float_t FT;
+  typedef double_t DT;
+  FT a;
+  DT b;
+  return a - b;
+}
+
+int foo5() {
+#pragma clang fp eval_method(source)
+  int t = _Generic( 1.0L, float_t:1, default:0);
+  int v = _Generic( 1.0L, double_t:1, default:0);
+  return t;
+}
+
+void foo6() {
+#pragma clang fp eval_method(source)
+  auto resf = [](float_t f) { return f; };
+  auto resd = [](double_t g) { return g; };
+}
+
+void foo7() {
+#pragma clang fp eval_method(source)
+  float f = (float_t)1; 
+  double d = (double_t)2; 
+}
+
+void foo8() {
+#pragma clang fp eval_method(source)
+  using Ft = float_t;
+  using Dt = double_t;
+  Ft a;
+  Dt b;
+}
+
+void foo9() {
+#pragma clang fp eval_method(source)
+  float c1 = (float_t)12;
+  double c2 = (double_t)13;
+}
+
+float foo10() {
+#pragma clang fp eval_method(source)
+  extern float_t f;
+  extern double_t g;
+  return f-g;
+}
+

clang/test/Sema/abi-check-2.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -DNOERROR %s
+// RUN: %clang_cc1 -fsyntax-only -verify -ffp-eval-method=double -DNOERROR %s
+
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=source %s
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=extended %s
+
+#ifdef NOERROR
+// expected-no-diagnostics
+typedef float float_t;
+typedef double double_t;
+#else
+typedef float float_t; //expected-error 9 {{cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit}}
+
+typedef double double_t; //expected-error 9 {{cannot use type 'double_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit}}
+#endif
+
+float foo1() {
+#pragma clang fp eval_method(double)
+  float a;
+  double b;
+  return a - b;
+}
+
+float foo2() {
+#pragma clang fp eval_method(double)
+  float_t a; 
+  double_t b; 
+  return a - b;
+}
+
+void foo3() {
+#pragma clang fp eval_method(double)
+  char buff[sizeof(float_t)];
+  char bufd[sizeof(double_t)];
+  buff[1] = bufd[2];
+}
+
+float foo4() {
+#pragma clang fp eval_method(double)
+  typedef float_t FT;
+  typedef double_t DT;
+  FT a;
+  DT b;
+  return a - b;
+}
+
+int foo5() {
+#pragma clang fp eval_method(double)
+  int t = _Generic( 1.0L, float_t:1, default:0);
+  int v = _Generic( 1.0L, double_t:1, default:0);
+  return t;
+}
+
+void foo6() {
+#pragma clang fp eval_method(double)
+  auto resf = [](float_t f) { return f; };
+  auto resd = [](double_t g) { return g; };
+}
+
+void foo7() {
+#pragma clang fp eval_method(double)
+  float f = (float_t)1; 
+  double d = (double_t)2; 
+}
+
+void foo8() {
+#pragma clang fp eval_method(double)
+  using Ft = float_t;
+  using Dt = double_t;
+  Ft a;
+  Dt b;
+}
+
+void foo9() {
+#pragma clang fp eval_method(double)
+  float c1 = (float_t)12;
+  double c2 = (double_t)13;
+}
+
+float foo10() {
+#pragma clang fp eval_method(double)
+  extern float_t f;
+  extern double_t g;
+  return f-g;
+}

clang/test/Sema/abi-check-3.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -DNOERROR %s
+// RUN: %clang_cc1 -fsyntax-only -verify -ffp-eval-method=extended -DNOERROR %s
+
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=source %s
+// RUN: %clang_cc1 -verify -fsyntax-only -ffp-eval-method=double %s
+
+
+#ifdef NOERROR
+// expected-no-diagnostics
+typedef float float_t;
+typedef double double_t;
+#else
+typedef float float_t; //expected-error 9 {{cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit}}
+
+typedef double double_t; //expected-error 9 {{cannot use type 'double_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit}}
+#endif
+
+float foo1() {
+#pragma clang fp eval_method(extended)
+  float a;
+  double b;
+  return a - b;
+}
+
+float foo2() {
+#pragma clang fp eval_method(extended)
+  float_t a; 
+  double_t b; 
+  return a - b;
+}
+
+void foo3() {
+#pragma clang fp eval_method(extended)
+  char buff[sizeof(float_t)];
+  char bufd[sizeof(double_t)];
+  buff[1] = bufd[2];
+}
+
+float foo4() {
+#pragma clang fp eval_method(extended)
+  typedef float_t FT;
+  typedef double_t DT;
+  FT a;
+  DT b;
+  return a - b;
+}
+
+int foo5() {
+#pragma clang fp eval_method(extended)
+  int t = _Generic( 1.0L, float_t:1, default:0);
+  int v = _Generic( 1.0L, double_t:1, default:0);
+  return t;
+}
+
+void foo6() {
+#pragma clang fp eval_method(extended)
+  auto resf = [](float_t f) { return f; };
+  auto resd = [](double_t g) { return g; };
+}
+
+void foo7() {
+#pragma clang fp eval_method(extended)
+  float f = (float_t)1; 
+  double d = (double_t)2; 
+}
+
+void foo8() {
+#pragma clang fp eval_method(extended)
+  using Ft = float_t;
+  using Dt = double_t;
+  Ft a;
+  Dt b;
+}
+
+void foo9() {
+#pragma clang fp eval_method(extended)
+  float c1 = (float_t)12;
+  double c2 = (double_t)13;
+}
+
+float foo10() {
+#pragma clang fp eval_method(extended)
+  extern float_t f;
+  extern double_t g;
+  return f-g;
+}

clang/test/Sema/attr-only-in-default-eval.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+typedef float float_t [[clang::available_only_in_default_eval_method]];
+using double_t __attribute__((available_only_in_default_eval_method)) = double;
+
+// expected-error@+1{{'available_only_in_default_eval_method' attribute only applies to typedefs}}
+class  __attribute__((available_only_in_default_eval_method)) C1 {
+};
+// expected-error@+1{{'available_only_in_default_eval_method' attribute only applies to typedefs}}
+class  [[clang::available_only_in_default_eval_method]] C2 {
+};
+
+// expected-error@+1{{'available_only_in_default_eval_method' attribute only applies to typedefs}}
+struct [[clang::available_only_in_default_eval_method]] S1;
+// expected-error@+1{{'available_only_in_default_eval_method' attribute only applies to typedefs}}
+struct __attribute__((available_only_in_default_eval_method)) S2;
+
+// expected-error@+1{{'available_only_in_default_eval_method' attribute only applies to typedefs}}
+void __attribute__((available_only_in_default_eval_method)) foo();
+// expected-error@+1{{'available_only_in_default_eval_method' attribute cannot be applied to types}}
+void [[clang::available_only_in_default_eval_method]] goo();
+// expected-error@+1{{'available_only_in_default_eval_method' attribute cannot be applied to types}}
+void bar() [[clang::available_only_in_default_eval_method]];
+// expected-error@+1{{'available_only_in_default_eval_method' attribute only applies to typedefs}}
+void barz() __attribute__((available_only_in_default_eval_method));
+