aaron.ballman (Aaron Ballman)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 14 2013, 3:16 PM (296 w, 6 d)

Recent Activity

Tue, Nov 20

aaron.ballman added inline comments to D54555: [clang][slh] add attribute for speculative load hardening.
Tue, Nov 20, 1:07 PM
aaron.ballman added a comment to D54450: Get the correct range of tokens for preprocessor conditions.

Ping.

Tue, Nov 20, 6:53 AM
aaron.ballman accepted D52695: [clang][Parse] Diagnose useless null statements (PR39111).

Aside from some small nits, LGTM!

Tue, Nov 20, 6:49 AM
aaron.ballman accepted D54704: [clang-tidy] Don't generate incorrect fixes for class constructed from list-initialized arguments.

Currently the smart_ptr check (modernize-make-unique) generates the fixes that cannot compile for cases like below -- because brace list can not be deduced in make_unique.

class Bar { int a, b; };
class Foo { Foo(Bar); };
auto foo = std::unique_ptr<Foo>(new Foo({1, 2}));
Tue, Nov 20, 6:40 AM
aaron.ballman added inline comments to D54555: [clang][slh] add attribute for speculative load hardening.
Tue, Nov 20, 6:24 AM

Mon, Nov 19

aaron.ballman added a comment to D52695: [clang][Parse] Diagnose useless null statements (PR39111).

Sorry about the delayed review on this -- I had intended to provide comments earlier, but somehow this fell off my radar.

Mon, Nov 19, 6:12 AM
aaron.ballman added inline comments to D54404: Exclude matchers which can have multiple results.
Mon, Nov 19, 5:21 AM

Sun, Nov 18

aaron.ballman added inline comments to D54404: Exclude matchers which can have multiple results.
Sun, Nov 18, 9:17 AM
aaron.ballman closed D54246: [clang-tidy] Add the abseil-duration-factory-scale check.

I've commit in r347163. Thank you for the patch!

Sun, Nov 18, 8:43 AM · Restricted Project
aaron.ballman added a comment to D52835: [Diagnostics] Check integer to floating point number implicit conversions.

It looks like you removed a considerable amount of testing coverage; why?

Sun, Nov 18, 7:47 AM

Sat, Nov 17

aaron.ballman added a comment to D54246: [clang-tidy] Add the abseil-duration-factory-scale check.

@aaron.ballman I don't actually have the commit bit, can you commit this, or are we waiting for further review?

Sat, Nov 17, 9:31 AM · Restricted Project

Fri, Nov 16

aaron.ballman accepted D54555: [clang][slh] add attribute for speculative load hardening.

Would there be value in making this a type attribute that appertains to the function type so that you don't lose this protection when calling through function pointers (as you could then add the attribute to the function pointer type as well)? If the stack pointer value is changed as part of the function prologue, this may not be needed, but if the stack pointer value is changed on the calling side, perhaps this would add value?

IMO, no.

This is fundamentally a property of the definition of a function, not of a call of a function. Whether the function being called has SLH or not doesn't change how you call it in any way.

Fri, Nov 16, 4:26 PM
aaron.ballman accepted D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.

I think this check is in good shape for the initial commit. Additional functionality can be added incrementally.

Fri, Nov 16, 2:07 PM · Restricted Project
aaron.ballman added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Fri, Nov 16, 1:34 PM · Restricted Project
aaron.ballman accepted D54246: [clang-tidy] Add the abseil-duration-factory-scale check.

LGTM!

Fri, Nov 16, 1:30 PM · Restricted Project
aaron.ballman added inline comments to D53488: [clang-tidy] Improving narrowing conversions.
Fri, Nov 16, 10:14 AM · Restricted Project
aaron.ballman added a comment to D54555: [clang][slh] add attribute for speculative load hardening.

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Not entirely sure, but I don't think so.
The way this is implemented both for x86 (if I understood the code correctly) and how this most likely will be implemented for AArch64 (see https://reviews.llvm.org/D49069 for what I expect will become the basis to implement SLH for AArch64) is that an illegal value is put in the stack pointer to signal misspeculation having happened. So, in that respect, having that illegal value in the stack pointer is an ABI rule to communicate mis-speculation across function boundaries.
However, given that this is ABI only used on miss-speculated paths, and miss-speculated paths get corrected eventually, programs will keep on working correctly if functions calling each other don't follow the same ABI in this respect. Obviously, you'll lose some protection against cross-function call miss-speculation.

Fri, Nov 16, 6:59 AM
aaron.ballman added a comment to D54555: [clang][slh] add attribute for speculative load hardening.

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Fri, Nov 16, 6:41 AM
aaron.ballman added a comment to D54334: [AVR] Automatically link CRT and libgcc from the system avr-gcc.

I'm not certain if it will be possible to devise test cases for the two diagnostics I pointed out or not, but otherwise, this LGTM as far as the implementation goes. I don't know enough about AVR to sign off on whether the patch logic is correct or not.

Fri, Nov 16, 6:17 AM
aaron.ballman added inline comments to D54334: [AVR] Automatically link CRT and libgcc from the system avr-gcc.
Fri, Nov 16, 5:14 AM

Thu, Nov 15

aaron.ballman added a comment to D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang.

Missing sema tests that demonstrate the attribute can only appertain to functions and types, accepts no arguments, has the expected semantic behavior for typecasts to function pointers, etc.

Thu, Nov 15, 4:33 AM
aaron.ballman added a comment to D54547: PTH-- Remove feature entirely-.

Should likely add release notes about this.

Also might be worth sending a note to cfe-dev as a heads up and give folks some time to say "wait wait".

Thu, Nov 15, 4:27 AM

Wed, Nov 14

aaron.ballman added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Wed, Nov 14, 11:03 AM · Restricted Project
aaron.ballman added inline comments to D54246: [clang-tidy] Add the abseil-duration-factory-scale check.
Wed, Nov 14, 10:51 AM · Restricted Project
aaron.ballman added inline comments to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".
Wed, Nov 14, 8:43 AM · Restricted Project
aaron.ballman added inline comments to D53488: [clang-tidy] Improving narrowing conversions.
Wed, Nov 14, 8:32 AM · Restricted Project
aaron.ballman accepted D54258: [Clang] Fix pretty printing of CUDA address spaces.

In D54258#1297191, @Anastasia wrote:

I agree with the change itself... but it's quite annoying that such things can't be tested. :(

Yes, that's a pity :(

Wed, Nov 14, 8:19 AM
aaron.ballman added a comment to D54450: Get the correct range of tokens for preprocessor conditions.

Oh, and running the check-clang-tools target, this currently results in:

Failing Tests (3):
    Clang Tools :: modularize/ProblemsInconsistent.modularize
    Clang Tools :: pp-trace/pp-trace-conditional.cpp
    Clang Tools :: pp-trace/pp-trace-macro.cpp

Perhaps the pp-trace one just has to be updated for the new behavior. :-)

Wed, Nov 14, 7:44 AM
aaron.ballman updated the diff for D54450: Get the correct range of tokens for preprocessor conditions.

Updating based on review feedback.

Wed, Nov 14, 7:41 AM
aaron.ballman added a comment to D52835: [Diagnostics] Check integer to floating point number implicit conversions.

Reverted.
Ok, I will address your comments soon.

Wed, Nov 14, 6:39 AM
aaron.ballman added inline comments to D52835: [Diagnostics] Check integer to floating point number implicit conversions.
Wed, Nov 14, 6:21 AM
aaron.ballman added a comment to D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜.

Thanks for the review everyone!

Let me know if there are any further changes that you want me to make or any further action required on my part to land this 😁

Wed, Nov 14, 5:39 AM · Restricted Project
aaron.ballman added inline comments to D54404: Exclude matchers which can have multiple results.
Wed, Nov 14, 5:33 AM
aaron.ballman added inline comments to D53488: [clang-tidy] Improving narrowing conversions.
Wed, Nov 14, 5:31 AM · Restricted Project
aaron.ballman accepted D54453: Remove myself as owner of clang-query..

I'll go ahead and add myself as code owner. Peter can remove himself at his leisure.

Wed, Nov 14, 5:02 AM

Mon, Nov 12

aaron.ballman added inline comments to D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang.
Mon, Nov 12, 4:16 PM
aaron.ballman added inline comments to D53982: Output "rule" information in SARIF.
Mon, Nov 12, 4:10 PM
aaron.ballman added a comment to D54258: [Clang] Fix pretty printing of CUDA address spaces.

There are external tools (e.g. hipacc) that generate Clang AST. This AST uses LangAS annotations and emits incorrect memory space specifiers for CUDA when pretty-printed.

Mon, Nov 12, 4:08 PM
aaron.ballman added inline comments to D54246: [clang-tidy] Add the abseil-duration-factory-scale check.
Mon, Nov 12, 4:02 PM · Restricted Project
aaron.ballman added inline comments to D54405: Record whether a AST Matcher constructs a Node.
Mon, Nov 12, 3:54 PM
aaron.ballman accepted D54402: Extract method to allow re-use.

This is just a NFC change, which is normal to appear without tests. The consensus on IRC is that this is fine.

Mon, Nov 12, 3:52 PM
aaron.ballman added a comment to D54453: Remove myself as owner of clang-query..

Thank you for all of your hard work on clang-query!

Mon, Nov 12, 3:42 PM
aaron.ballman accepted D54407: Record the matcher type when storing a binding.

Aside from a minor formatting nit, LGTM.

Mon, Nov 12, 3:33 PM
aaron.ballman added inline comments to D54408: Add matchers available through casting to derived.
Mon, Nov 12, 3:21 PM
aaron.ballman added a comment to D54407: Record the matcher type when storing a binding.

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

Mon, Nov 12, 3:08 PM
aaron.ballman accepted D54406: Add matchDynamic convenience functions.

LGTM!

Mon, Nov 12, 3:03 PM
aaron.ballman added a comment to D54405: Record whether a AST Matcher constructs a Node.

Aside from the uses of auto and the lack of tests, this LGTM.

Mon, Nov 12, 3:02 PM
aaron.ballman added a reviewer for D54404: Exclude matchers which can have multiple results: sbenza.

I acknowledge and share the future-proofing concern.

We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.

Mon, Nov 12, 2:57 PM
aaron.ballman accepted D54403: Add new API for returning matching matchers.

LGTM aside from a minor commenting nit.

Mon, Nov 12, 2:49 PM
aaron.ballman added a comment to D54402: Extract method to allow re-use.

I think this commit is fine without tests.

Mon, Nov 12, 2:46 PM
aaron.ballman created D54450: Get the correct range of tokens for preprocessor conditions.
Mon, Nov 12, 2:31 PM

Sun, Nov 11

aaron.ballman added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Sun, Nov 11, 6:34 PM · Restricted Project
aaron.ballman added a reviewer for D54407: Record the matcher type when storing a binding: sbenza.

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

Sun, Nov 11, 3:57 PM
aaron.ballman added inline comments to D54406: Add matchDynamic convenience functions.
Sun, Nov 11, 3:51 PM
aaron.ballman added inline comments to D54405: Record whether a AST Matcher constructs a Node.
Sun, Nov 11, 3:47 PM
aaron.ballman added inline comments to D54404: Exclude matchers which can have multiple results.
Sun, Nov 11, 3:40 PM
aaron.ballman added inline comments to D54403: Add new API for returning matching matchers.
Sun, Nov 11, 3:32 PM
aaron.ballman added a comment to D54402: Extract method to allow re-use.

Generally LGTM but this has no test coverage. What are your plans for how you want to see this proceed? Do you intend to commit everything in one batch once all of the reviews are accepted, or do piecemeal commits?

Sun, Nov 11, 3:19 PM
aaron.ballman added a comment to D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜.

Generally LGTM, but I leave it to someone more well-versed in ObjC to sign off that this actually does everything those users would expect.

Sun, Nov 11, 2:53 PM · Restricted Project
aaron.ballman added inline comments to D54246: [clang-tidy] Add the abseil-duration-factory-scale check.
Sun, Nov 11, 2:45 PM · Restricted Project

Sat, Nov 10

aaron.ballman accepted D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

LGTM aside from some small commenting nits. However, I leave it to @erik.pilkington to make the final sign-off.

Sat, Nov 10, 2:53 PM
aaron.ballman added a comment to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif

#if defined(FOO)
#if defined(FOO)
#endif
#endif

#if !defined(FOO)
#if !defined(FOO)
#endif
#endif

I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO == 4' string (the condition) from the If() callback. So far I only see how to get the start location of the condition and the whole range till the end of endif. Do you have a code pointer how to do this, please? (Or OK if I leave this for a follow-up patch?)

Sat, Nov 10, 12:54 PM · Restricted Project
aaron.ballman added a comment to D52984: [analyzer] Checker reviewer's checklist.
  • Move the checklist up before additional info in the HTML file.
  • Fix minor nits.
  • Add missing bullet points (thanks @Szelethus for noticing)

    I did not add any coding convention related item yet. I wonder if it is a good idea to have our own coding guidelines even if it is derived from the LLVM one.
Sat, Nov 10, 7:20 AM · Restricted Project

Fri, Nov 9

aaron.ballman added a comment to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif
Fri, Nov 9, 4:25 PM · Restricted Project
aaron.ballman added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

Have you tried running creduce on the preprocessed source? We should really have a real reproducer for this, otherwise its really hard to tell what the underlying problem is.

Fri, Nov 9, 1:33 PM
aaron.ballman added inline comments to D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜.
Fri, Nov 9, 1:04 PM · Restricted Project
aaron.ballman accepted D54307: [ASTMatchers] overload ignoringParens for Expr.

LGTM!

Fri, Nov 9, 11:52 AM · Restricted Project
aaron.ballman added reviewers for D54307: [ASTMatchers] overload ignoringParens for Expr: sbenza, klimek.

Adding a few other reviewers in case they have ideas on how to use the polymorphic matcher rather than the overload. If they don't have a better idea, then I think the overload approach is fine.

Fri, Nov 9, 11:34 AM · Restricted Project
aaron.ballman added inline comments to D54246: [clang-tidy] Add the abseil-duration-factory-scale check.
Fri, Nov 9, 10:49 AM · Restricted Project
aaron.ballman added inline comments to D54307: [ASTMatchers] overload ignoringParens for Expr.
Fri, Nov 9, 10:13 AM · Restricted Project
aaron.ballman added inline comments to D53488: [clang-tidy] Improving narrowing conversions.
Fri, Nov 9, 10:05 AM · Restricted Project
aaron.ballman added a comment to D54258: [Clang] Fix pretty printing of CUDA address spaces.

I think it's not so easy to provide such tests for CUDA.
CUDA memory space specifiers are implemented via attributes, e.g. #define __shared__ __attribute__((shared)).
As a result of this, they are pretty-printed via a different code path.
In my example, I call Ctx.getAddrSpaceQualType(QT, LangAS::cuda_shared), which is then pretty-printed via the code above.
Any hints how to provide tests for this one?

Fri, Nov 9, 9:54 AM
aaron.ballman closed D53700: Support _Clang as a scoped attribute identifier.

Thanks for the reviews -- I've commit in r346521.

Fri, Nov 9, 9:22 AM
aaron.ballman accepted D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay.

LGTM aside from a minor commenting nit.

Fri, Nov 9, 8:53 AM
aaron.ballman added a comment to D54307: [ASTMatchers] overload ignoringParens for Expr.

Also: the patch is missing unit tests.

Fri, Nov 9, 8:39 AM · Restricted Project
aaron.ballman added inline comments to D54307: [ASTMatchers] overload ignoringParens for Expr.
Fri, Nov 9, 8:39 AM · Restricted Project

Thu, Nov 8

aaron.ballman added inline comments to D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay.
Thu, Nov 8, 4:04 PM
aaron.ballman added inline comments to D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay.
Thu, Nov 8, 3:50 PM
aaron.ballman added inline comments to D54222: [clang-tidy] Add a check to detect returning static locals in public headers.
Thu, Nov 8, 3:44 PM · Restricted Project
aaron.ballman added inline comments to D54222: [clang-tidy] Add a check to detect returning static locals in public headers.
Thu, Nov 8, 2:39 PM · Restricted Project
aaron.ballman added inline comments to D54262: [clang-tidy] Add `delete this` bugprone check (PR38741).
Thu, Nov 8, 11:05 AM · Restricted Project
aaron.ballman added inline comments to D53488: [clang-tidy] Improving narrowing conversions.
Thu, Nov 8, 10:52 AM · Restricted Project
aaron.ballman added inline comments to D54246: [clang-tidy] Add the abseil-duration-factory-scale check.
Thu, Nov 8, 10:18 AM · Restricted Project
aaron.ballman added a comment to D54258: [Clang] Fix pretty printing of CUDA address spaces.

Can you add tests for this change? We typically have these in Misc by passing -ast-print.

Thu, Nov 8, 10:05 AM

Wed, Nov 7

aaron.ballman added inline comments to D54222: [clang-tidy] Add a check to detect returning static locals in public headers.
Wed, Nov 7, 3:09 PM · Restricted Project
aaron.ballman added inline comments to D54169: [clang-tidy] Zircon <fbl/limits.h> -> <limits>.
Wed, Nov 7, 11:50 AM · Restricted Project

Tue, Nov 6

aaron.ballman added inline comments to D54169: [clang-tidy] Zircon <fbl/limits.h> -> <limits>.
Tue, Nov 6, 7:04 PM · Restricted Project
aaron.ballman added inline comments to D54169: [clang-tidy] Zircon <fbl/limits.h> -> <limits>.
Tue, Nov 6, 6:49 PM · Restricted Project
aaron.ballman added inline comments to D54169: [clang-tidy] Zircon <fbl/limits.h> -> <limits>.
Tue, Nov 6, 1:24 PM · Restricted Project
aaron.ballman added a comment to D52421: [Sema] Diagnose parameter names that shadow inherited field names.

And now i'm having concerns.

struct Base {
    void* f;
};

struct Inherit : Base {
    static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in the 'Base'? You can't access that non-static member variable from static function.
    }
};
Tue, Nov 6, 9:43 AM

Mon, Nov 5

aaron.ballman added reviewers for D53700: Support _Clang as a scoped attribute identifier: mclow.lists, EricWF, ldionne.

Another option that @rsmith and I discussed today is perhaps using __clang or clang__ as the identifier, but perhaps this will cause more confusion about where to put underscores than _Clang would. If there are other ideas that are more palatable for the identifier, that'd be great -- I am not strongly tied to a spelling.

Mon, Nov 5, 1:28 PM
aaron.ballman added inline comments to D53982: Output "rule" information in SARIF.
Mon, Nov 5, 7:59 AM
aaron.ballman added inline comments to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.
Mon, Nov 5, 7:48 AM
aaron.ballman added inline comments to D53982: Output "rule" information in SARIF.
Mon, Nov 5, 7:36 AM

Sun, Nov 4

aaron.ballman added inline comments to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.
Sun, Nov 4, 12:17 PM
aaron.ballman added inline comments to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.
Sun, Nov 4, 7:54 AM

Fri, Nov 2

aaron.ballman closed D52421: [Sema] Diagnose parameter names that shadow inherited field names.

I've commit in r346041.

Fri, Nov 2, 2:08 PM

Thu, Nov 1

aaron.ballman added inline comments to D52984: [analyzer] Checker reviewer's checklist.
Thu, Nov 1, 2:28 PM · Restricted Project
aaron.ballman closed D53982: Output "rule" information in SARIF.

Thank you for the quick review and feedback -- I've commit in r345874.

Thu, Nov 1, 12:00 PM
aaron.ballman added inline comments to D53982: Output "rule" information in SARIF.
Thu, Nov 1, 11:42 AM