In D146595#4225702, @aprantl wrote:

In D146595#4225630, @aaron.ballman wrote:

It's less about other debug formats and more about user experience. My two big concerns are: 1) writing multiple attributes to do the same thing is somewhat user-hostile unless "the same thing" is actually different in some way users will care about, 2) we have a policy that user-facing attributes that do nothing are diagnosed as being ignored, so if we don't support Windows for this attribute, then compiling on that platform is going to generate a pile of "attribute ignored" warnings. If we can support the attribute with PDB as well, that solves both of my big concerns because it makes this attribute generally useful (which is what we aim for with attributes when possible).

That's a valid concern. However, in its current form it would still get lowered into LLVM IR, and LLVM may or may not silently ignore the attribute when lowering to PDB. It could be argued that that's even worse than warning about an ignored attribute. I just wanted to point this out.

Thanks for working on this fix! A few things:

I don't think the _BitInt test case should block this patch if that turns out to cause problems.

In D146412#4227644, @Fznamznon wrote:

Should OutputStream be made into a shared_ptr so we can clarify the memory ownership instead of leaving it as a raw pointer? That should also address the use-after-free.

Well, I was thinking about that and wasn't sure about it either. https://reviews.llvm.org/D129456 added initialization of OutputStream from the outside. When last shared_ptr is destroyed, it destroys undelying object as well. In case underlying object came from the outside in a form of raw pointer, we may end up accidentally deleting it when DumpModuleInfoAction is destroyed.

In D146973#4227470, @jhuber6 wrote:

In D146973#4227433, @aaron.ballman wrote:

I am not asking you to implement a library based off another implementation's specification. I am relaying implementation experience with the design you've chosen for your implementation and how well it's worked in other, related projects. Given that two different technologies have both run into this same problem, I think the llvm-libc folks should carefully consider the design decisions here. If it turns out this is the best way forward, that's fine.

Sorry, that was more directed at Johannes, This is definitely a hard problem. Each approach has certain benefits, but I think keeping the headers synced like we do in OpenMP has mainly worked thus far because we don't have any actual implementations of most of it. If we want to provide a library I don't think there's a reasonable way to implement it as a unified header unless we control the system header as well. I'm hoping that libc offers a sufficiently small surface that we should be able to provide functionality that's expected for both. And in some cases it should be fine to share existing headers, but it shouldn't be the expected route it all I'm saying.

I'm not asking you to copy other libc headers. I'm pointing out that having two separate headers, one for host and one for device, is a recipe for problems in practice because these two will invariably get out of sync in really fascinating ways that are extremely hard for people to debug. But maybe there's a misunderstanding here: I am assuming we consider it to be unsupported to use glibc/musl/etc on the host and llvm-libc on the device, but maybe that's a faulty assumption.

We can't do that for the time being, since LLVM's libc is still in development. It's not a sufficient replacement for the host libc at this point. It may be an interesting point to get to in the future, it would make it much easier to keep things in sync for sure. It may be easier to stipulate something like that with libc++ when we get to that point since libc++ is more complete as far as I'm aware.

In D146412#4223272, @Fznamznon wrote:

In D146412#4220021, @kastiglione wrote:

I understand the potential for a use after free, since OutputStream is a raw pointer, but I don't understand the fix.

Okay, probably commit message is a little confusing. The potential for a use after free is that OutputStream is a raw pointer as you already noted, but it is also accessible outside of DumpModuleInfoAction::ExecuteAction. Before the patch there was the case where result of local unique_ptr::get was saved there. When the function is exited, unique_ptr frees the memory it holds, making OutputStream poiniting to freed memory. Since OutputStream can be easily accessed outside of DumpModuleInfoAction::ExecuteAction, hence the potential for use-after-free. I wasn't sure if assigning nullptr to OutputStream at the end of DumpModuleInfoAction::ExecuteAction was a good idea, so I just added code avoiding saving result of unique_ptr::get to OutputStream .

LGTM, though the change should come with a release note. Suggestion you can take or leave as you see fit: should we turn the places where we removed the null pointer check into assertions that the pointer is nonnull? Or are we hoping the crash will be sufficient to tell us when we've missed a case?

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.

In D146973#4227266, @jhuber6 wrote:

In D146973#4227114, @aaron.ballman wrote:

Hmmm, I've had experience with SYCL as to how it goes when you have difference between host and device; those kinds of bugs are incredibly hard to track down. Pointer sizes being compatible is a good start, but you need integer widths, floating point formats, structure layout decisions, macro definitions, etc to all be the same as well. Having only one set of headers that can be used helps users avoid these sort of problems.

The problem is that we are trying to implement an actual library here. It is, in my opinion, completely unreasonable to try to implement a library based off of another implementation's specification.

This will also need someone from the LLVM side to look at the LLVM-specific changes. Most of my comments were focused on the behavior of test cases, but there may be more comments coming for the code changes once I've got a better handle on the test behavior.

In D146973#4225645, @jhuber6 wrote:

In D146973#4225641, @aaron.ballman wrote:

This lets offloading languages such as OpenMP use the system string.h when compiling for the host and then the LLVM libc string.h when targeting the GPU.

How do we avoid ABI issues when the two headers get sufficiently out of sync? (In general, I'm pretty surprised to hear we would want different headers for the GPU and the system -- does this affect conformance requirements from the C standard?)

I'm not entirely sure if there's a good method. I think no matter what we do we'll need to implement some kind of 'glue'. I think most should be fine if we go by the C-standard. We expect pointer sizes and everything to be compatible at least.

In D146986#4225275, @dblaikie wrote:

Presumably adding an alias for #pragma clang system_header called system_module wouldn't be too hard? (though the pragma is also being removed from libc++ soon, I think, in favor of -isystem usage, so maybe that's a sign the pragma's not a great way to do)

This lets offloading languages such as OpenMP use the system string.h when compiling for the host and then the LLVM libc string.h when targeting the GPU.

In D146595#4225490, @aprantl wrote:

We don't want to have one attribute per debug format, because that's not going to scale.

LLVM supports outputting a total of 2 debug info formats.

In D146595#4225259, @augusto2112 wrote:

In D146595#4225170, @aaron.ballman wrote:

In D146595#4225132, @aprantl wrote:

In D146595#4225048, @dblaikie wrote:

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.

This patch aims at exposing an existing LLVM IR & DWARF feature in clang. Having a generic solution for the std::function problem is definitely worthwhile investigating, but I'm not sure it needs to prevent supporting the existing mechanism in clang.

Why should this feature be limited to DWARF? Don't we have the same kind of stepping behavior issue with PDB files, for example?

There's already support to threading trampoline names from IR to DWARF. If PDB supports the same attribute in some form there's nothing stopping someone to adding support to thread the attribute from IR to PDB as well.

In D146986#4225121, @dblaikie wrote:

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.

(hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))

In D146595#4225132, @aprantl wrote:

In D146595#4225048, @dblaikie wrote:

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.

This patch aims at exposing an existing LLVM IR & DWARF feature in clang. Having a generic solution for the std::function problem is definitely worthwhile investigating, but I'm not sure it needs to prevent supporting the existing mechanism in clang.

In D144358#4225139, @Aditya-pixel wrote:

Oh, okay, Ill change the patch now. Also, it seems my local directory is out of sync. Should I always update it before submitting a patch, or is it alright without updating it?

In D144358#4225124, @aaron.ballman wrote:

In D144358#4225123, @Aditya-pixel wrote:

In D144358#4225034, @aaron.ballman wrote:

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

I'll fix that one up and land it as an NFC change, so you don't need to worry about that. Thanks for pointing it out though!

In D144358#4225123, @Aditya-pixel wrote:

In D144358#4225034, @aaron.ballman wrote:

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

LGTM! Precommit CI is still falling over because of the libcxx bot. That issue is being actively investigated, but we don't have an ETA on it being resolved. I think these changes are sufficiently safe to land and watch for post-commit failures.

I think this LGTM, but I'm holding off on signing off until @shafik is satisfied with the part he was asking about. You should add a release note for the fix, too.

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

In D146867#4224567, @simideveloper wrote:

but I think this patch https://reviews.llvm.org/D146358 has not been accepted yet so you should give me a chance.

The changes here LGTM, but I'd appreciate it if a second set of eyes double-checked whether I missed anything, given the complexity of the changes. (So wait a day or so before landing in case other reviewers want to chime in.)

In D146520#4224483, @PiotrZSL wrote:

In D146520#4224281, @aaron.ballman wrote:

Ideally, users should be able to disable the core checks from running at all (the reason users want to disable checks is both because of the diagnostics they produce and because of the cost of running the check itself).

I'm trying to enable this here: https://reviews.llvm.org/D146396, but problem is that I'm not sure what core checkers should be enabled by default. Simply Static Analysis config file does not show that.

LGTM, but you should fix up the commit message when you land since the patch has changed away from what's described in the summary.

In D146520#4221314, @carlosgalvezp wrote:

Reading through Github I found the associated ticket (it would be good if you could mention it in the commit message):
https://github.com/llvm/llvm-project/issues/61520

So if I understand correctly, what you are trying to do is continue to let the clang-analyzer-core checks run, and simply ignore the warnings coming from them when enabling warnings_as_errors. I'm not entirely convinced that's the best approach, it's hiding the real problem - that as a user one cannot disable clang-analyzer-core checks (which is being fixed in another patch by PiotrZSL). I also find strange to move error-handling logic away from the DiagnosticConsumer class out to the main ClangTidy class.

Would be good if @njames93 or @aaron.ballman could comment on this, given their experience.

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

In D146945#4223702, @Pierre-vh wrote:

In D146945#4223677, @aaron.ballman wrote:

LGTM! Adding the clang-vendors group for awareness since this technically could break some downstream (I don't expect it to given that this was deprecated, but you never know).

Can it land or should I wait for someone from that group to comment first?
Thanks

(Btw, if you no longer plan to work on a patch, it's considered good form to "abandon" the patch in Phabricator. You can do this by clicking the Add Action drop-down menu and selecting Abandon Changes.)

LGTM! Adding the clang-vendors group for awareness since this technically could break some downstream (I don't expect it to given that this was deprecated, but you never know).

In D145671#4223674, @Pierre-vh wrote:

In D145671#4223655, @c wrote:

These changes need a release note. Given that this has been deprecated, is there documentation that should have been removed as well?

I can't find anything in the docs about code-object-v3 so I don't think there's any documentation left. For the release note, I opened a diff: D146945

These changes need a release note. Given that this has been deprecated, is there documentation that should have been removed as well?

Fixing a typo pointed out off-list (missed an "as").

Ping

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.

LGTM!

Want to add a release note for this as well?

In D144651#4219633, @fdeazeve wrote:

In D144651#4219340, @aaron.ballman wrote:

The build lab's lldb builders are all green, and greendragon's standalone build is also green. Could this be a problem with incremental builds being in a bad state somehow?

I don't think so, as I can reliably reproduce this on my machine. I believe the _contents_ of the SDK headers affect what is fed to Clang, and therefore we see the crash with some SDKs but not others.

In D146784#4219469, @tomasz-kaminski-sonarsource wrote:

This is now tracked as https://github.com/llvm/llvm-project/issues/61680. Let me know if I should refer it in the test.

LGTM with a suggestion.

The changes LGTM (though I had a small nit), but you should add a release note when landing.

In D144651#4217750, @john.brawn wrote:

Unfortunately I still can't reproduce this even using exactly the same cmake command as you gave. Looking at above cmake log some possible causes of difference are:

• I'm doing this on an M1 macbook with host triple arm64-apple-darwin21.6.0 but the host triple on that bot is x86_64-apple-darwin20.6.0
• I'm using python 3.9, bot is using python 3.10
• I'm doing a build from clean whereas the bot is doing an incremental build

Also if I try to run the simpler reproducer you give then I get the error

error: expression failed to parse:
error: Header search couldn't locate module Cocoa

I'm now on holiday, I won't be able to get back to this until 2023-04-03.

LGTM! (The changes need a release note, though.) Do you need someone to commit this on your behalf? If so, please let me know what name and email address you would like used for patch attribution. I can add the release note when I land, or you can update the patch to add a release note, either is fine by me.

LGTM!

This should also have a release note, eventually.

In D146503#4217541, @chaitanyav wrote:

Thank you, I will commit myself. Should i wait for the build to finish?. this will be my first direct commit to repo.

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

In D141472#4112790, @tbaeder wrote:

I'm not sure what to do about this right now. I was wondering about restructuring pointers so all the metadata is before all the actual data, but that would be a large refactoring.

LGTM with a bit more testing.

LGTM

In D146466#4213814, @nickdesaulniers wrote:

In D146466#4210607, @aaron.ballman wrote:

In D146466#4208216, @dblaikie wrote:

If the issue is with the actual fallthrough - that's been discussed elsewhere recently, and I think it'd be reasonable to turn on TrapOnUnreachable everywhere, basically (with NoTrapAfterNoreturn too, perhaps, to keep the cost low) so that these trivial fallthroughs hit int3 instead of going on to the next function.

As I understand things, folks are confused about the runtime behavior of falling through and magically calling some other function. If we can warn about that situation from the FE without false positives, then huttah, I think that's sufficient. But I don't think we know of a way to do that from the FE, so I think ensuring a runtime trap is more helpful to folks than the status quo. (Also, in general, I think we want to avoid emitting diagnostics from the backend -- that has usability issues where clean -fsyntax-only builds suddenly get warnings when doing full codegen, or in circumstances where the optimization level changes the diagnostic output, etc.)

The issue is that there are problems that may occur as a result of optimizations; if the front end does not do optimizations, it cannot diagnose these. Some examples include:

• -Wframe-larger-than= (useful for embedded systems without "nearly unlimited" stack, though an imperfect proxy for maximal stack depth, which is what these systems actually want. Helpful for spotting large stack allocations that should come from the heap or static memory).
• -Wattribute-warning (some implementations of _FORTIFY_SOURCE such as the Linux kernel's rely on this; specifically that calls to functions with these attributes get optimized out. This provides significant protections during compile time when due to optimizations we have an out of bounds access, when compared to implementations that simply use _Static_assert)
• -Wfunction-fallthrough (hypothetical new diagnostic this patch is adding)

Even if Clang had a high level IR and did optimizations, it still could not guarantee the results of LLVM's subsequent optimizations.

Does that mean that -Wframe-larger-than= (and the like) should not exist because it's not diagnosable via -fsyntax-only?

I do think that there are a class of diagnostics that are useful to users that depend on optimizations being run. While a policy around new diagnostics being diagnosable via -fsyntax-only is a simplistic litmus test, I do think it allows for our competition to implement diagnostics that may be more useful to users. This hamstrings our competitiveness, IMO.

LGTM aside from a tiny variable naming nit I missed before.

In D145201#4214395, @cjdb wrote:

swaps commits to see if that fixes CI (part 1)

In D139705#4216449, @tomasz-kaminski-sonarsource wrote:

In D139705#4216417, @erichkeane wrote:

In D139705#4215653, @tomasz-kaminski-sonarsource wrote:

As a downstream, we have concerns with this change. From what I saw it breaks the behavior of the fix-it that wants to remove the whole variable definition (including the initializer). For example, I have unused, that I want to remove variable x and unnecessary explicit specialization temp<double>:

template<typename T>
T temp = 1;

int x  = 10;
template<> double temp<double> = 1;

Previously, this could be handled (in case of single variable declaration) by simply removing up the var->getSourceRange() with succeeding coma. However, after the change, such code does not work, and in general if getSourceRange() is used on VarDecl (or even Decl), special consideration needs to be taken for the situation when VarDecl refers to variable template specialization.

As an alternative, I would suggest introducing an additional function to VarDecl (which could be moved up in the hierarchy), that would report a source range that corresponds to declarator-id, i.e. for template variable it would include template arguments.

Hmm... I'm being a little dense here I guess, I don't understand what you mean? Does this result in one of our fixits being wrong here? With the old range, wouldn't it have left the <double> in that case, and now is removing it? Or what am I missing?

Before the change, for both x and temp<double>, prior to the change the getSourceRange() on the VarDecl that represents them includes an initializer (they end just before ;). But now for the variable template specialization, we end up just after template arguments. This means that fixit/report that previously covered the whole definition, will now not include an initializer.
Or in our example:

template<typename T>
T temp = 1;
     // v getSourceRange() ends on this token before and after the change
int x = 10;
                              // v getSourceRange() ends on this token prior to the change, consistently with normal variables
template<> double temp<double> = 1;
                          // ^ getSourceRange() ends on this token after the change, making it inconsistent

The precommit CI failures are unrelated to your changes (the Debian one is related to flang failures and the libcxx one seems to be related to infrastructure), so no need to worry about them.

In D146497#4216278, @Hahnfeld wrote:

In D146497#4216226, @aaron.ballman wrote:

The issue doesn't reproduce for me locally on my Windows machine, so it may be something specific to the VE setup.

FWIW I'm not doing anything specific for VE, just

set(CMAKE_C_COMPILER "clang" CACHE STRING "")
set(CMAKE_CXX_COMPILER "clang++" CACHE STRING "")
set(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "")
set(LLVM_APPEND_VC_REV OFF CACHE STRING "")
set(LLVM_ENABLE_ASSERTIONS ON CACHE BOOL "")
set(LLVM_ENABLE_PROJECTS "clang" CACHE STRING "")

set(LLVM_ENABLE_LLD ON CACHE BOOL "")
set(LLVM_LINK_LLVM_DYLIB ON CACHE BOOL "")

in the file I load as the initial-cache (cmake -C).

In D146634#4215754, @kadircet wrote:

I am aware that this null checking at leaves are not considered a sustainable solution and I agree with the sentiment there. But we're seeing an increasing number of crashes in production on invalid code recently. Happy to take a different course if there are alternatives, but as also explained in D146426, the situation around parameter lists having nullptrs seem to be the state for a long time now, e.g:

template <typename T> auto x = [](__fp16) {};
decltype(x<int>);

is a reproducer that crashes even clang-12 due to a nullptr in the paremeter list. Surely it'd be better to fix this invariant, but I am afraid we don't know how to do that immediately and considering people have been dealing with this situation by adding null checks into the places that triggered crashes ever since, I'd like to move forward with this fix until someone can figure out the situation.

In D146497#4216197, @aaron.ballman wrote:

In D146497#4215650, @Hahnfeld wrote:

FYI this commit breaks clang/test/Index/index-file.cu for me:

warning: CUDA version 11.8 is only partially supported [-Wunknown-cuda-version]
warning: CUDA version 11.8 is only partially supported [-Wunknown-cuda-version]
/home/jhahnfel/LLVM/src/clang/test/Index/index-file.cu:8:20: error: CHECK-HOST-NOT: excluded string found in input
// CHECK-HOST-NOT: macro definition=__CUDA_ARCH__
                   ^
<stdin>:6218:48: note: found here
// CHECK: __clang_cuda_runtime_wrapper.h:70:9: macro definition=__CUDA_ARCH__ Extent=[70:9 - 70:27]
                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It could be related to the warning about my locally installed CUDA version, but __CUDA_ARCH__ should really not be defined when compiling for the host. If you have any idea what's going on here, please let me know - I'm a bit puzzled right now...

This is confirmed on at least this build bot: https://lab.llvm.org/buildbot/#/builders/91/builds/15271

I'll investigate locally and revert if it's not something I can fix forward.

In D146497#4215650, @Hahnfeld wrote:

FYI this commit breaks clang/test/Index/index-file.cu for me:

warning: CUDA version 11.8 is only partially supported [-Wunknown-cuda-version]
warning: CUDA version 11.8 is only partially supported [-Wunknown-cuda-version]
/home/jhahnfel/LLVM/src/clang/test/Index/index-file.cu:8:20: error: CHECK-HOST-NOT: excluded string found in input
// CHECK-HOST-NOT: macro definition=__CUDA_ARCH__
                   ^
<stdin>:6218:48: note: found here
// CHECK: __clang_cuda_runtime_wrapper.h:70:9: macro definition=__CUDA_ARCH__ Extent=[70:9 - 70:27]
                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It could be related to the warning about my locally installed CUDA version, but __CUDA_ARCH__ should really not be defined when compiling for the host. If you have any idea what's going on here, please let me know - I'm a bit puzzled right now...

LGTM

LGTM!

I can see some minor utility to this for some kinds of libraries perhaps, but I'm on the fence about adding the attribute. Is there a reason we need the user to write this attribute at all? e.g., could we be smarter about inline function definitions rather than making this the user's problem?

In D146530#4213076, @ipriyanshi1708 wrote:

In D146530#4213045, @aaron.ballman wrote:

This appears to be the same efforts that have been going on in https://reviews.llvm.org/D146041. Can you coordinate with @AryanGodara so that there's only one patch for this?

Hi! I am an Outreachy applicant, firstly how can I coordinate with @AryanGodara and secondly how will it be counted as my contribution and then can I record it in my final application? Or I will have to look for some other issues for Outreachy?

This appears to be the same efforts that have been going on in https://reviews.llvm.org/D146041. Can you coordinate with @AryanGodara so that there's only one patch for this?

Where do you suggest I add the documentation?

Please also add a release note and add documentation for the builtin.

Thanks for this!

(Note, precommit CI on Windows still shows a valid failure.)

You should also add a release note to clang/docs/ReleaseNotes.rst so users know about the new functionality.

LGTM

In D146466#4208216, @dblaikie wrote:

If the issue is with the actual fallthrough - that's been discussed elsewhere recently, and I think it'd be reasonable to turn on TrapOnUnreachable everywhere, basically (with NoTrapAfterNoreturn too, perhaps, to keep the cost low) so that these trivial fallthroughs hit int3 instead of going on to the next function.

LGTM with a nit and a question, but can you add a release note when you land to let users know about the changes?

In D144358#4208735, @Aditya-pixel wrote:

Hello! I have added the diff file after running the clang-format command. However, it outputs no files changed. Is the patch I updated formatted?

I am getting this message.