Page MenuHomePhabricator

aaron.ballman (Aaron Ballman)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 14 2013, 3:16 PM (523 w, 5 d)

Recent Activity

Today

aaron.ballman updated subscribers of D146595: [clang] Add clang trampoline_mangled_target attribute.

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.

Tue, Mar 28, 1:05 PM · debug-info, Restricted Project, Restricted Project, Restricted Project
aaron.ballman added a reviewer for D147037: [Clang][ICE] Corrected invalid invalid parameter index on some attributes with invalid indices applied to varargs functions: erichkeane.

Thanks for working on this fix! A few things:

Tue, Mar 28, 12:49 PM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D145545: [clang][Interp] Fix local variable (destructor) management in loop bodies.
Tue, Mar 28, 12:29 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146408: [clang][Interp] Start supporting complex types.

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

Tue, Mar 28, 12:23 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146412: [NFC] Fix potential use-after-free in DumpModuleInfoAction::ExecuteAction().

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.

Tue, Mar 28, 11:02 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146973: [Clang] Implicitly include LLVM libc headers for the GPU.

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.

Tue, Mar 28, 10:25 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146412: [NFC] Fix potential use-after-free in DumpModuleInfoAction::ExecuteAction().

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 .

Tue, Mar 28, 7:49 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146971: [Sema] Populate declarations inside TypeLocs for some invalid types.

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?

Tue, Mar 28, 7:37 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method.

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.

Tue, Mar 28, 7:26 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146973: [Clang] Implicitly include LLVM libc headers for the GPU.

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.

Tue, Mar 28, 7:11 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D139010: [clang][WebAssembly] Implement support for table types and builtins.

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.

Tue, Mar 28, 6:35 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman committed rGe574833c2bc4: Downgrade reserved module identifier error into a warning (authored by aaron.ballman).
Downgrade reserved module identifier error into a warning
Tue, Mar 28, 5:54 AM · Restricted Project, Restricted Project
aaron.ballman closed D146986: Downgrade reserved module identifier error into a warning.
Tue, Mar 28, 5:54 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146973: [Clang] Implicitly include LLVM libc headers for the GPU.

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.

Tue, Mar 28, 5:22 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146986: Downgrade reserved module identifier error into a warning.

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)

Tue, Mar 28, 4:54 AM · Restricted Project, Restricted Project

Yesterday

aaron.ballman added a comment to D146973: [Clang] Implicitly include LLVM libc headers for the GPU.

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.

Mon, Mar 27, 2:51 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146595: [clang] Add clang trampoline_mangled_target attribute.

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.

Mon, Mar 27, 2:37 PM · debug-info, Restricted Project, Restricted Project, Restricted Project
aaron.ballman added a comment to D146595: [clang] Add clang trampoline_mangled_target attribute.

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.

Mon, Mar 27, 12:34 PM · debug-info, Restricted Project, Restricted Project, Restricted Project
aaron.ballman added inline comments to D145545: [clang][Interp] Fix local variable (destructor) management in loop bodies.
Mon, Mar 27, 12:15 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146986: Downgrade reserved module identifier error into a warning.

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

Mon, Mar 27, 12:00 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146595: [clang] Add clang trampoline_mangled_target attribute.

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.

Mon, Mar 27, 11:52 AM · debug-info, Restricted Project, Restricted Project, Restricted Project
aaron.ballman added a comment to D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].

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?

Mon, Mar 27, 11:49 AM · Restricted Project, Restricted Project
aaron.ballman committed rGdedd7b6548f4: Added checking for completeness of lvalue in conditional operator (authored by Aditya-pixel).
Added checking for completeness of lvalue in conditional operator
Mon, Mar 27, 11:47 AM · Restricted Project, Restricted Project
aaron.ballman closed D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].
Mon, Mar 27, 11:47 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].

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!

Mon, Mar 27, 11:41 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].

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>`)
Mon, Mar 27, 11:39 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146801: [clang] Fix consteval initializers of temporaries.

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.

Mon, Mar 27, 11:22 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146234: [clang] Fix crash when handling nested immediate invocations.

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.

Mon, Mar 27, 11:18 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].

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.

Mon, Mar 27, 11:03 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146764: [clang] Make predefined expressions string literals under -fms-extensions.
Mon, Mar 27, 10:57 AM · Restricted Project, Restricted Project
aaron.ballman requested review of D146986: Downgrade reserved module identifier error into a warning.
Mon, Mar 27, 9:58 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146867: [Diagnostic] printing name of uninitialized subobject instead of its type.

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

Mon, Mar 27, 8:57 AM · Restricted Project, Restricted Project
aaron.ballman accepted D145737: PR60985: Fix merging of lambda closure types across modules..

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

Mon, Mar 27, 8:51 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146604: [NFC] Fix uninitalized member variable use in ASTReader::ParseTargetOptions().
Mon, Mar 27, 8:33 AM · Restricted Project, Restricted Project
aaron.ballman updated subscribers of D146520: [clang-tidy] Fix checks filter with warnings-as-errors.

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.

Mon, Mar 27, 8:26 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146847: [NFC] Fix uninitialized member variable use in RVVEmitter::createRVVIntrinsics().

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.

Mon, Mar 27, 8:24 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146520: [clang-tidy] Fix checks filter with warnings-as-errors.

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.

Mon, Mar 27, 7:44 AM · Restricted Project, Restricted Project
aaron.ballman committed rGce548b6130b6: Document the Clang policies on claiming support for a feature (authored by aaron.ballman).
Document the Clang policies on claiming support for a feature
Mon, Mar 27, 6:58 AM · Restricted Project, Restricted Project
aaron.ballman closed D146420: Document the Clang policies on claiming support for a feature.
Mon, Mar 27, 6:58 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method.

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

Mon, Mar 27, 6:47 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146945: Add Release Note for -mcode-object-v3 removal.

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

Mon, Mar 27, 4:39 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146867: [Diagnostic] printing name of uninitialized subobject instead of its type.

(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.)

Mon, Mar 27, 4:33 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146945: Add Release Note for -mcode-object-v3 removal.

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

Mon, Mar 27, 4:29 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D145671: [clang] Remove legacy -m(no)-code-object-v3 options.
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

Mon, Mar 27, 4:28 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D145671: [clang] Remove legacy -m(no)-code-object-v3 options.

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

Mon, Mar 27, 4:21 AM · Restricted Project, Restricted Project

Sat, Mar 25

aaron.ballman added inline comments to D146441: [APFloat] Add E4M3B11FNUZ.
Sat, Mar 25, 7:56 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
aaron.ballman added inline comments to D146719: [Clang] Improve diagnostics when using a concept as template argument.
Sat, Mar 25, 5:38 AM · Restricted Project, Restricted Project
aaron.ballman updated the diff for D146420: Document the Clang policies on claiming support for a feature.

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

Sat, Mar 25, 5:27 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146420: Document the Clang policies on claiming support for a feature.

Ping

Sat, Mar 25, 5:20 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146441: [APFloat] Add E4M3B11FNUZ.
Sat, Mar 25, 5:19 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Fri, Mar 24

aaron.ballman added a comment to D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method.

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.

Fri, Mar 24, 12:23 PM · Restricted Project, Restricted Project
aaron.ballman accepted D141472: [clang][Interp] Add function pointers.

LGTM!

Fri, Mar 24, 9:23 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146719: [Clang] Improve diagnostics when using a concept as template argument.

Want to add a release note for this as well?

Fri, Mar 24, 9:09 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144651: [Serialization] Place command line defines in the correct file.

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.

Fri, Mar 24, 8:25 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman added inline comments to D146764: [clang] Make predefined expressions string literals under -fms-extensions.
Fri, Mar 24, 8:21 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146408: [clang][Interp] Start supporting complex types.
Fri, Mar 24, 7:56 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146784: [clang] Test for AST Dumping of the template variables.

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

Fri, Mar 24, 7:26 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146801: [clang] Fix consteval initializers of temporaries.
Fri, Mar 24, 7:15 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146788: [clang][Interp] Fix zero-initializing of floating types.

LGTM with a suggestion.

Fri, Mar 24, 7:11 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146784: [clang] Test for AST Dumping of the template variables.
Fri, Mar 24, 7:01 AM · Restricted Project, Restricted Project
aaron.ballman accepted D146801: [clang] Fix consteval initializers of temporaries.

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

Fri, Mar 24, 6:39 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144651: [Serialization] Place command line defines in the correct file.

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.

Fri, Mar 24, 6:30 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman accepted D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].

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.

Fri, Mar 24, 6:23 AM · Restricted Project, Restricted Project
aaron.ballman accepted D144115: [clang] Extend pragma dump to support expressions.

LGTM!

Fri, Mar 24, 6:20 AM · Restricted Project, Restricted Project
aaron.ballman requested changes to D146764: [clang] Make predefined expressions string literals under -fms-extensions.

This should also have a release note, eventually.

Fri, Mar 24, 6:03 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146503: Fix highlighting issue with _complex and initialization list with more than 2 items.

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

Fri, Mar 24, 5:53 AM · Restricted Project, Restricted Project
aaron.ballman committed rGef310c6d4980: Fix Clang sphinx build (authored by aaron.ballman).
Fix Clang sphinx build
Fri, Mar 24, 4:28 AM · Restricted Project, Restricted Project

Thu, Mar 23

aaron.ballman accepted D146503: Fix highlighting issue with _complex and initialization list with more than 2 items.

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

Thu, Mar 23, 12:36 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D141472: [clang][Interp] Add function pointers.

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.

Thu, Mar 23, 12:08 PM · Restricted Project, Restricted Project
aaron.ballman accepted D145860: [clang][Interp] Fix initializing fields after base class members.

LGTM with a bit more testing.

Thu, Mar 23, 11:47 AM · Restricted Project, Restricted Project
aaron.ballman accepted D145841: [clang][Interp] Fix diagnostics for calling non-constexpr constructors.

LGTM

Thu, Mar 23, 10:53 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D146408: [clang][Interp] Start supporting complex types.
Thu, Mar 23, 10:28 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146466: [clang] diagnose function fallthrough.

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.

Thu, Mar 23, 10:02 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman accepted D145201: [clang] fixes header processing for `-fdiagnostics-format=sarif`.

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

Thu, Mar 23, 7:21 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D145201: [clang] fixes header processing for `-fdiagnostics-format=sarif`.

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

Thu, Mar 23, 7:19 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D139705: [clang] fix zero-initialization fix-it for variable template.

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
Thu, Mar 23, 6:46 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146503: Fix highlighting issue with _complex and initialization list with more than 2 items.

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.

Thu, Mar 23, 5:55 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146497: libclang: Pass Clang install directory to driver via argv[0]..

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

Thu, Mar 23, 5:35 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146634: [clang][USR] Prevent crashes when parameter lists have nulls.

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.

Thu, Mar 23, 5:22 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman added a comment to D146497: libclang: Pass Clang install directory to driver via argv[0]..

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.

Thu, Mar 23, 5:08 AM · Restricted Project, Restricted Project
aaron.ballman added a reverting change for rG201fdef40dd6: libclang: Pass Clang install directory to driver via argv[0].: rG18d56880a89a: Revert "libclang: Pass Clang install directory to driver via argv[0].".
Thu, Mar 23, 5:06 AM · Restricted Project, Restricted Project
aaron.ballman committed rG18d56880a89a: Revert "libclang: Pass Clang install directory to driver via argv[0]." (authored by aaron.ballman).
Revert "libclang: Pass Clang install directory to driver via argv[0]."
Thu, Mar 23, 5:06 AM · Restricted Project, Restricted Project
aaron.ballman added a reverting change for D146497: libclang: Pass Clang install directory to driver via argv[0].: rG18d56880a89a: Revert "libclang: Pass Clang install directory to driver via argv[0].".
Thu, Mar 23, 5:06 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146497: libclang: Pass Clang install directory to driver via argv[0]..

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

Thu, Mar 23, 4:47 AM · Restricted Project, Restricted Project

Wed, Mar 22

aaron.ballman accepted D146535: [Clang] Fix evaluation of parameters of lambda call operator attributes.

LGTM

Wed, Mar 22, 12:17 PM · Restricted Project, Restricted Project
aaron.ballman accepted D146155: [clang][NFC] Fix location of 2>&1 in a few -print tests.

LGTM!

Wed, Mar 22, 11:05 AM · Restricted Project, Restricted Project
aaron.ballman requested changes to D146595: [clang] Add clang trampoline_mangled_target attribute.

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?

Wed, Mar 22, 10:47 AM · debug-info, Restricted Project, Restricted Project, Restricted Project
aaron.ballman added a comment to D146530: [clang][diagnostics]Removed "sorry" from all the required files.

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?

Wed, Mar 22, 6:54 AM · Restricted Project, Restricted Project
aaron.ballman updated subscribers of D146530: [clang][diagnostics]Removed "sorry" from all the required files.

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?

Wed, Mar 22, 6:43 AM · Restricted Project, Restricted Project
aaron.ballman added reviewers for D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method: rjmccall, serge-sans-paille.

Where do you suggest I add the documentation?

Wed, Mar 22, 6:16 AM · Restricted Project, Restricted Project
aaron.ballman updated subscribers of D146513: Add missing nearbyintf16 builtin functions.

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

Wed, Mar 22, 6:11 AM · Restricted Project
aaron.ballman added inline comments to D144115: [clang] Extend pragma dump to support expressions.
Wed, Mar 22, 6:02 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146503: Fix highlighting issue with _complex and initialization list with more than 2 items.

Thanks for this!

Wed, Mar 22, 5:56 AM · Restricted Project, Restricted Project

Tue, Mar 21

aaron.ballman added a comment to D145201: [clang] fixes header processing for `-fdiagnostics-format=sarif`.

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

Tue, Mar 21, 12:19 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144115: [clang] Extend pragma dump to support expressions.

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

Tue, Mar 21, 12:11 PM · Restricted Project, Restricted Project
aaron.ballman accepted D146436: [clang][Interp][NFC] Add tests for __fp16.

LGTM

Tue, Mar 21, 11:42 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D146466: [clang] diagnose function fallthrough.

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.

Tue, Mar 21, 11:41 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman accepted D146497: libclang: Pass Clang install directory to driver via argv[0]..

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

Tue, Mar 21, 11:08 AM · Restricted Project, Restricted Project
aaron.ballman committed rGb904e68f13ba: No longer issue static lambda pedantic warning for pre-c++2b compat (authored by aaron.ballman).
No longer issue static lambda pedantic warning for pre-c++2b compat
Tue, Mar 21, 9:50 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D144358: [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].

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.

Tue, Mar 21, 8:53 AM · Restricted Project, Restricted Project