[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288132. jfb added a comment. As Vedant suggested, make it part of 'integer' sanitizer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/include/clang/Basic/Sa

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288133. jfb added a comment. Re-upload with full context. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/docs/UndefinedBehaviorSanitizer.rst clang/include

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288383. jfb marked 6 inline comments as done. jfb added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ ht

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884 llvm::Value *BitsShiftedOff = Builder.CreateLShr( Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros", /*NUW*/ true, /*NSW*/

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288388. jfb marked an inline comment as done. jfb added a comment. Fix notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/docs/UndefinedBehaviorSanitizer

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + lebedev.ri wrote: > Surely not `~1U`. Indeed, fixed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288444. jfb marked 4 inline comments as done. jfb added a comment. Remove the "suppress this" in release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: cla

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6 + volatile unsigned _v = (val);\ + volatile unsigned _a = (amount); \ + unsigned res = _v << _a; \ vsk wrote: > jfb wrote: > > vsk wrote: > > > Doe

[PATCH] D86000: Add an unsigned shift base sanitizer

This revision was automatically updated to reflect the committed changes. Closed by commit rG82d29b397bb2: Add an unsigned shift base sanitizer (authored by jfb). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D860

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

jfb requested changes to this revision. jfb added a comment. This revision now requires changes to proceed. Herald added a subscriber: dexonsmith. Please consider these changes, and whether this is relevant as implemented: http://wg21.link/p0270 Comment at: clang-tools-extra/

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

jfb requested changes to this revision. jfb added a comment. This revision now requires changes to proceed. MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270. Additionally

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/m

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jfb added a comment. On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch. Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:805-817 + return IsASCII ? "^" : (const char *)u8"\u2548"; case LineChar::Ra

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

jfb added a comment. Most of the tests as written should be failing right now, at least on macOS and Linux, because they likely should be identified as POSIX, right? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75229/new/ https://reviews.llvm.org/

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/m

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

jfb added a comment. Let's make sure that we follow the same semantics that GCC does, particularly w.r.t. union, bitfields, and padding at the end of an object (whether it's in an array or not). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87974/

[PATCH] D108469: Improve handling of static assert messages.

jfb added a comment. Can you test all the values in this? https://godbolt.org/z/h7n54fa5x Comment at: clang/lib/Basic/Diagnostic.cpp:792 +static void pushEscapedString(StringRef Str, SmallVectorImpl &OutStr) { + OutStr.reserve(OutStr.size() + Str.size()); + const unsigned cha

[PATCH] D104975: Implement P1949

jfb added a comment. It would be more user friendly to say which character is not allowed in the diagnostic. Do we need to have a backwards compatible flag to preserve the old behavior, or do we believe that nobody will be affected by the change? We should make this choice explicitly and state

[PATCH] D155580: [trivial-auto-var-init] Do not emit initialization code for empty class

jfb added a comment. (sorry for sending twice, looks like email reply failed) This is the same as padding, and is initialized on purpose. If it’s truly unused then the optimizer will eliminate it… unless it can’t prove whether the store is dead. Does this show up in any meaningless performance

[PATCH] D76323: [AST] Fix handling of long double and bool in __builtin_bit_cast

jfb added a comment. Maybe you should test `nullptr` as well, given that it's all padding? Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:407 + +constexpr bool test_bad_bool = bit_cast('A'); // expected-error {{must be initialized by a constant expression}} expe

[PATCH] D76528: [clang codegen] Clean up handling of vectors with trivial-auto-var-init.

jfb added a comment. Oh good catch. Were there no vector tests which tripped this? Vectors can hold scalar types with padding (i.e. `long double`)? Tail padding in vectors do seem important to initialize as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D76528: [clang codegen] Clean up handling of vectors with trivial-auto-var-init.

jfb added a comment. The test files I added checked initialization had stores to each padding location, I think that's what would be needed here as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76528/new/ https://reviews.llvm.org/D76528 _

[PATCH] D76528: [clang codegen] Clean up handling of vectors with trivial-auto-var-init.

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76528/new/ https://reviews.llvm.org/D76528 ___ cf

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. What's the verdict then? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68115/new/ https://reviews.llvm.org/D68115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. In D68115#1946757 , @rsmith wrote: > In D68115#1946668 , > @hubert.reinterpretcast wrote: > > > It sounds like we are looking for `-fzero-union-padding`. That's been where > > the discussion h

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. I'm not sure this is a good idea at all. We want to keep the codepath as simple as possible to avoid introducing bugs. If a codebase sees a crash then it's easier to bisect one function at a time than doing something like this. I'd much rather see bisection using pragma to

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. I'd much rather see folks bisect using something like: void use(void*); #pragma clang attribute push ([[clang::uninitialized]], apply_to = variable) void buggy() { int arr[256]; int boom; float bam; struct { int oops; } oops; union { int

[PATCH] D77219: UBSan ␇ runtime

jfb created this revision. Herald added subscribers: Sanitizers, cfe-commits, ributzka, dexonsmith, jkorous, cryptoad, mgorny. Herald added projects: clang, Sanitizers. jfb edited the summary of this revision. Yes, this is April 1st and the patch isn't particularly serious. There is a ␇ UBSan ru

[PATCH] D77219: UBSan ␇ runtime

jfb marked an inline comment as done. jfb added inline comments. Comment at: compiler-rt/lib/ubsan_bel/ubsan_bel_handlers.cpp:40-42 +static std::random_device r; +static std::default_random_engine e(r()); +static std::uniform_int_distribution d(0, sizeof(quips) / siz

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. Do you not think `pragma` is a more general approach? That's what's used in a bunch of other cases, and I'd like to see it attempted here. If not, I agree with John that just counting up isn't a good bisection experience. I'd rather see a begin / end bound. You're also miss

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. In D77168#1955450 , @jcai19 wrote: > In D77168#1955312 , @jfb wrote: > > > Do you not think `pragma` is a more general approach? That's what's used in > > a bunch of other cases, and I'd like t

[PATCH] D77311: clang-format: [JS] detect C++ keywords.

jfb added a comment. Some of these are technically "future reserved keywords": https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Keywords Wouldn't it be better to list all of JS's keywords at this point? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

jfb added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:1193 + // the given cpu and returns `0` if the CPU is not found. + virtual unsigned getCPUCacheLineSize() const { return 0; } + Return an optional instead of using zero to mean "unkno

[PATCH] D63131: arm64_32: implement the desired ABI for the ILP32 triple.

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. Minor comments, but otherwise LGTM. Comment at: clang/lib/Basic/Targets/AArch64.cpp:167 // Target properties. - if (!getTriple().isOSWind

[PATCH] D63131: arm64_32: implement the desired ABI for the ILP32 triple.

jfb added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5457 // for AArch64, emit a warning and ignore the flag. Otherwise, add the // proper mllvm flags. + if (Triple.getArch() != llvm::Triple::aarch64 && t.p.northover wro

[PATCH] D71091: Make sure that the implicit arguments for direct methods have been setup

jfb added a comment. You should upload patches with context :) Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4030 const ObjCContainerDecl *CD) { - auto I = DirectMethodDefinitions.find(OMD); + auto I = DirectMethodDefinitions.find(OMD->get

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jfb added subscribers: ldionne, EricWF, mclow.lists. jfb added a comment. This generally seems fine. Does it work on most backends? I want to make sure it doesn't fail in backends :) Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for floating-point atomic support? CHANGES

[PATCH] D76077: [ARM] Add __bf16 as new Bfloat16 C Type

jfb added a comment. In D76077#1921973 , @rjmccall wrote: > In D76077#1921490 , @LukeGeeson > wrote: > > > In D76077#1919861 , @rjmccall > > wrote: > > > > > I don't unders

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

jfb added a comment. In D74361#1879559 , @rjmccall wrote: > I'm not sure if it's a good idea to be re-using the existing attribute like > this. It's not that unreasonable, I guess, but I'd like to hear JF's > thoughts. The current uninitialized attrib

[PATCH] D67399: [ARM] Follow AACPS for preserving number of loads/stores of volatile bit-fields

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. I'm happy with this change since it's opt-in. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67399/new/ https://reviews.llvm.org/D67399

[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

jfb added a comment. Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL implement `is_always_lock_free` at the moment? CL 19 2017 RTW doesn't seem to have anything ? Presumably they'll have to do *something*. https://reviews.llvm.org/D3

[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

jfb added a comment. In https://reviews.llvm.org/D32265#731710, @EricWF wrote: > In https://reviews.llvm.org/D32265#731709, @jfb wrote: > > > Is it a goal to support Microsoft's STL with this? If so, how does MSVC's > > STL implement `is_always_lock_free` at the moment? CL 19 2017 RTW doesn't >

[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

jfb added a comment. It looks like Billy is going to do something somewhat similar when he rewrites it: https://twitter.com/jfbastien/status/855168230918307840 For now it's kinda `#define IS_LOCK_FREE ¯\_(ツ)_/¯` https://reviews.llvm.org/D32265 ___

[PATCH] D115440: Provide __builtin_alloca*_uninitialized variants

jfb added a comment. In D115440#3183778 , @melver wrote: > GCC devs say that initializing explicit alloca() is a bug, because they > aren't "automatic storage": > https://lkml.kernel.org/r/20211209201616.gu...@gate.crashing.org > .. which is also the re

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Maybe refer to http://wg21.link/p0418 directly in the commit message? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98995/new/ https://reviews.llvm.org/D98995 _

[PATCH] D100057: Remove warning "suggest braces" for aggregate initialization of an empty class with an aggregate base class.

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. A few nits, but this is good otherwise! Comment at: clang/lib/Sema/SemaInit.cpp:1013 - auto *ParentRD = - Entity.getParent()->getType()->castAs()->getDecl(); - if (CXXReco

[PATCH] D108469: Improve handling of static assert messages.

jfb added a subscriber: hwright. jfb added a comment. I worry that changing the general `static_assert` printing (adding a colon, and dropping the quotes) will get @hwright's law to drop on us. We can try and see if e.g. users of clang have automated checks for `static_assert` in their CI pipel

<    1   2   3   4   5   6