Page MenuHomePhabricator

rupprecht (Jordan Rupprecht)
Engineering

Projects

User does not belong to any projects.

User Details

User Since
Jun 28 2018, 11:39 AM (209 w, 4 d)

Recent Activity

Feb 10 2022

rupprecht committed rGd6b1448809e4: [libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with… (authored by rupprecht).
[libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with…
Feb 10 2022, 9:04 AM
rupprecht closed D118950: [libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with `_LIBCPP_DEBUG_LEVEL`.
Feb 10 2022, 9:04 AM · Restricted Project
rupprecht added a comment to D118950: [libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with `_LIBCPP_DEBUG_LEVEL`.

This looks correct to me, but thanks for waiting. The debug mode is sometimes tricky, but in this case this really looks like an oversight. Basically, _LIBCPP_DEBUG is the user-facing option, and the library should use _LIBCPP_DEBUG_LEVEL internally.

However, the commit summary says "Remove _LIBCPP_DEBUG and replace with _LIBCPP_DEBUG_LEVEL", which is misleading. I thought you were removing _LIBCPP_DEBUG entirely at first. I think this would be better: "Remove usage of _LIBCPP_DEBUG in __comp_ref_type" or something along those lines.

Feb 10 2022, 9:02 AM · Restricted Project
rupprecht retitled D118950: [libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with `_LIBCPP_DEBUG_LEVEL` from [libc++] Remove `_LIBCPP_DEBUG` and replace with `_LIBCPP_DEBUG_LEVEL` to [libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with `_LIBCPP_DEBUG_LEVEL`.
Feb 10 2022, 9:01 AM · Restricted Project
rupprecht committed rG99e5c5256ff2: [libc++] Fix std::__debug_less in c++17. (authored by rupprecht).
[libc++] Fix std::__debug_less in c++17.
Feb 10 2022, 8:54 AM
rupprecht closed D118940: [libc++] Fix std::__debug_less in c++17..
Feb 10 2022, 8:54 AM · Restricted Project

Feb 3 2022

rupprecht added a comment to D118940: [libc++] Fix std::__debug_less in c++17..

The patch looks good to me now... but we do have a buildkite runner that runs in Debug mode, and it didn't catch the thing you originally mentioned.
Can someone explain why libcxx/test/std/algorithms/alg.sorting/alg.min.max/min_init_list_comp.pass.cpp does not detect the problem? I paste it into Godbolt and it detects the problem: https://godbolt.org/z/eKvcvbb7d yet the "Debug mode" buildkite job isn't failing.

...Ooh. It's because the "Debug mode" buildkite job uses only C++20 or C++2b, isn't it? The combinatorial explosion of config options strikes again. :( @ldionne @philnik any thoughts on how to improve our CI coverage here?

Yeah, exponential growth really sucks. A few things I thought of:
(1) Have an infinite amount of money to throw at CI. That would mean we have 0 bugs, but it might not be feasible.
(2) Ignore it and hope for the best

Also seems the most reasonable IMHO. Breakages seem pretty rare, I don't know if it's worth a bunch of resources for a once-a-year bug.

Feb 3 2022, 8:00 PM · Restricted Project
rupprecht requested review of D118950: [libc++] Remove usage of `_LIBCPP_DEBUG` in `__comp_ref_type` and replace with `_LIBCPP_DEBUG_LEVEL`.
Feb 3 2022, 3:08 PM · Restricted Project
rupprecht added a comment to D118940: [libc++] Fix std::__debug_less in c++17..

As philnik said, please revert all the extraneous changes. (And please ignore clang-format's noise going forward. We'd disable clang-format entirely if we could. This is an ongoing pain point for libc++, over and over and over.)

IIUC, the only intentional functional change here is to change _LIBCPP_CONSTEXPR_AFTER_CXX17 into _LIBCPP_CONSTEXPR_AFTER_CXX11, is that right? That certainly seems like a good idea to me. (My mantra there is that internal details should always use the most aggressive constexpr possible. That way, if we ever drop support for C++03 and C++11 modes, the amount of stuff can be search-and-replaced to plain constexpr immediately is maximized.)

That's correct -- supporting this for C++17 was the only change I care about.

Feb 3 2022, 2:34 PM · Restricted Project
rupprecht updated the diff for D118940: [libc++] Fix std::__debug_less in c++17..
  • Restoring original version of the patch
Feb 3 2022, 2:26 PM · Restricted Project
rupprecht updated the diff for D118940: [libc++] Fix std::__debug_less in c++17..
  • Undo clang-formatting
Feb 3 2022, 2:23 PM · Restricted Project
rupprecht added inline comments to D118940: [libc++] Fix std::__debug_less in c++17..
Feb 3 2022, 1:40 PM · Restricted Project
rupprecht updated the diff for D118940: [libc++] Fix std::__debug_less in c++17..
  • Use _LIBCPP_HIDE_FROM_ABI
  • Use `_LIBCPP_DEBUG_LEVEL
  • Fix clang-format CI test failure
  • Fix whitespace in test
Feb 3 2022, 1:38 PM · Restricted Project
rupprecht requested review of D118940: [libc++] Fix std::__debug_less in c++17..
Feb 3 2022, 12:40 PM · Restricted Project

Jan 31 2022

rupprecht added a comment to D118671: [Bazel] Don't fail the build on usage of deprecated APIs.

SGTM

Jan 31 2022, 7:04 PM · Restricted Project

Jan 28 2022

rupprecht committed rG282c83c32384: [libc] Add missing sqrt deps for layering checks (authored by rupprecht).
[libc] Add missing sqrt deps for layering checks
Jan 28 2022, 12:12 PM

Jan 26 2022

rupprecht committed rGe7cf10958703: [bazel] Enable layering_check for MLIR test directory (authored by rupprecht).
[bazel] Enable layering_check for MLIR test directory
Jan 26 2022, 1:40 PM
rupprecht closed D118125: [bazel] Enable layering_check for MLIR test directory.
Jan 26 2022, 1:40 PM · Restricted Project

Jan 25 2022

rupprecht committed rG493509a40ad1: [NFC] DeclCXX: Fix -Wreorder-ctor (authored by rupprecht).
[NFC] DeclCXX: Fix -Wreorder-ctor
Jan 25 2022, 2:30 PM
rupprecht requested review of D118125: [bazel] Enable layering_check for MLIR test directory.
Jan 25 2022, 4:44 AM · Restricted Project

Jan 24 2022

rupprecht committed rG71cb5ed03c9b: [bazel] Update MLIR deps (authored by rupprecht).
[bazel] Update MLIR deps
Jan 24 2022, 9:25 PM
rupprecht added a comment to D118053: [libc] Add bazel definition for hypot/hypotf..

LGTM. @lntue is planning to refactor the hypot setup so I have added him to the list of reviewers.

Jan 24 2022, 3:23 PM · Restricted Project
rupprecht committed rGd4be9720e7e6: [test] Fix no-undef-type-md.ll. (authored by rupprecht).
[test] Fix no-undef-type-md.ll.
Jan 24 2022, 12:04 PM
rupprecht committed rG57eb5033cdff: [libc] Add bazel definition for hypot/hypotf. (authored by cratonica).
[libc] Add bazel definition for hypot/hypotf.
Jan 24 2022, 9:55 AM
rupprecht closed D118053: [libc] Add bazel definition for hypot/hypotf..
Jan 24 2022, 9:55 AM · Restricted Project
rupprecht requested review of D118053: [libc] Add bazel definition for hypot/hypotf..
Jan 24 2022, 9:45 AM · Restricted Project

Jan 10 2022

rupprecht updated the diff for D114722: [LLDB] Fix Python GIL-not-held issues.

Rebase

Jan 10 2022, 8:25 AM · Restricted Project

Dec 3 2021

rupprecht committed rGfddedcaeb8de: [NFC] const-ify some methods on CommandReturnObject (authored by rupprecht).
[NFC] const-ify some methods on CommandReturnObject
Dec 3 2021, 2:55 PM

Nov 29 2021

rupprecht requested review of D114722: [LLDB] Fix Python GIL-not-held issues.
Nov 29 2021, 9:58 AM · Restricted Project

Nov 11 2021

rupprecht committed rGda4822f6c815: [PowerPC][NFC] Ignore unused var in release builds. (authored by rupprecht).
[PowerPC][NFC] Ignore unused var in release builds.
Nov 11 2021, 8:59 AM

Nov 10 2021

rupprecht added a reverting change for rG3bf96b0329be: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls: rG360d901bf047: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 10 2021, 10:39 AM
rupprecht committed rG360d901bf047: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls" (authored by rupprecht).
Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls"
Nov 10 2021, 10:39 AM
rupprecht added a reverting change for D102993: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls: rG360d901bf047: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 10 2021, 10:39 AM · Restricted Project
rupprecht closed D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 10 2021, 10:39 AM · Restricted Project
rupprecht added a comment to D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".

I actually didn't see that the patch deleted the TestCppReferenceToOuterClass test. Could you just add a @skipIf # Crashes or so above its def test... method? The test itself is still valid user code that we shouldn't crash on.

I assume you mean something like expectedFailure, as this should unconditionally fail.

Nov 10 2021, 10:29 AM · Restricted Project
rupprecht updated the diff for D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
  • Use better test APIs, add test back w/ a @expectedFailure
Nov 10 2021, 10:27 AM · Restricted Project
rupprecht updated the diff for D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
  • Switch crash repro from shell -> api test
Nov 10 2021, 3:45 AM · Restricted Project

Nov 9 2021

rupprecht added inline comments to D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 9 2021, 4:56 PM · Restricted Project
rupprecht added a comment to D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".

Given that this patch has been in tree for half a year, it'd be good to get confirmation here this can be reverted given there is now a test case for causing a crash. I got an offline comment that this is OK to revert, so if nobody has objections, I'll land sometime tomorrow.

Nov 9 2021, 2:29 PM · Restricted Project

Nov 8 2021

rupprecht added a reverting change for rG3bf96b0329be: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls: D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 8 2021, 6:28 PM
rupprecht requested review of D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 8 2021, 6:28 PM · Restricted Project
rupprecht added a reverting change for D102993: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls: D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls".
Nov 8 2021, 6:28 PM · Restricted Project

Nov 5 2021

rupprecht added inline comments to D113214: [IR][ShuffleVector] Introduce `isReplicationMask()` matcher.
Nov 5 2021, 2:50 PM · Restricted Project
rupprecht added inline comments to D113214: [IR][ShuffleVector] Introduce `isReplicationMask()` matcher.
Nov 5 2021, 8:12 AM · Restricted Project
rupprecht added inline comments to D113214: [IR][ShuffleVector] Introduce `isReplicationMask()` matcher.
Nov 5 2021, 7:58 AM · Restricted Project
rupprecht added inline comments to D113214: [IR][ShuffleVector] Introduce `isReplicationMask()` matcher.
Nov 5 2021, 7:50 AM · Restricted Project

Nov 3 2021

rupprecht added a comment to D110867: X86InstrInfo: Support immediates that are +1/-1 different in optimizeCompareInstr.

This version of the patch LGTM with regards to non-determinism

Nov 3 2021, 11:38 AM · Restricted Project
rupprecht added a comment to D110867: X86InstrInfo: Support immediates that are +1/-1 different in optimizeCompareInstr.

I've reverted back to green in https://github.com/llvm/llvm-project/commit/a2a58d91e82db38fbdf88cc317dcb3753d79d492 until this can be fixed.

FWIW, we also saw a non-determinism issue as a result of this patch in a stage2 PGO'd build of clang.

I believe the new version of this diff fixes the reported issue. Do you have a way to confirm?

Nov 3 2021, 11:09 AM · Restricted Project
rupprecht added inline comments to D112732: [ASan] Process functions in Asan module pass.
Nov 3 2021, 11:05 AM · Restricted Project, Restricted Project
rupprecht added inline comments to D112732: [ASan] Process functions in Asan module pass.
Nov 3 2021, 11:02 AM · Restricted Project, Restricted Project
rupprecht added a comment to D110867: X86InstrInfo: Support immediates that are +1/-1 different in optimizeCompareInstr.

FWIW, we also saw a non-determinism issue as a result of this patch in a stage2 PGO'd build of clang.

Nov 3 2021, 9:49 AM · Restricted Project

Sep 22 2021

rupprecht added a comment to D108389: [libc++] Bypass calling exception-throwing functions in the dylib with -fno-exceptions.

(Additionally, its errors are more useful, too, e.g. showing the actual out of range index, which makes that all the more valuable to emit in both modes).

100% agree here, our errors suck. I think there's a bug report to improve that somewhere.

[We experienced some churn from this commit, but so far just seems like brittle tests]

Please let me know if you experience anything serious from this change. I would expect it's NFC for 99.9% of people, except if you were doing something shady. If that's not correct, I might learn something useful so please let me know.

Sep 22 2021, 11:11 AM · Restricted Project

Aug 30 2021

rupprecht added a comment to D108389: [libc++] Bypass calling exception-throwing functions in the dylib with -fno-exceptions.

To answer your question more directly, the goal here in terms of behavior is to ensure that a TU using std::vector::at (and friends) under -fno-exceptions doesn't rely on a weak-def symbol located in the dylib -- which it technically doesn't need cause all we do is abort() anyway.

This does not seem like a desirable change.

Previously, if you compiled libc++ itself with exceptions (as is generally the case), then a call to std::vector::at from a TU with -fno-exceptions _does_ still throw an exception. Of course, this exception cannot validly unwind through that TU, so it will eventually call std::terminate -- but not before printing its error message (e.g. "terminating with uncaught exception of type std::out_of_range: vector").

Switching to call abort() directly is a degradation in functionality, for no real gain, AFAICT.

The only two classes that have this behavior are std::string and std::vector. All other classes define their exception-throwing functions in the headers, so they end up calling abort() directly. That looks accidental to me, and not really something we'd want to consider a contract with our users. WDYT?

Aug 30 2021, 3:11 PM · Restricted Project

Aug 17 2021

rupprecht added a comment to D108147: [SamplePGO][NFC] Dump function profiles in order.

FYI, I ran into this commit when pulling into our downstream fork because we had some tests comparing expected profile dump output. Usually labeling a commit as "NFC" is reserved for patches that have no externally visible effect, but this changes the output of llvm-profdata show. See https://lists.llvm.org/pipermail/llvm-dev/2021-June/151234.html for recent discussion. I can certainly see this being a bit of an edge case.

Aug 17 2021, 6:08 PM · Restricted Project

Aug 16 2021

rupprecht updated subscribers of rG45138f788c9b: [sanitizer] Define 32bit uptr as uint.

Looks like reverting this is causing different build errors, like

/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/lib/sanitizer_common/tests/../sanitizer_allocator_size_class_map.h:198:50: error: format specifies type 'size_t' (aka 'unsigned int') but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Werror,-Wformat]
          i, Size(i), d, p, l, MaxCachedHint(s), cached, ClassID(s));
                                                 ^~~~~~
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/lib/sanitizer_common/tests/../sanitizer_allocator_size_class_map.h:198:58: error: format specifies type 'size_t' (aka 'unsigned int') but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Werror,-Wformat]
          i, Size(i), d, p, l, MaxCachedHint(s), cached, ClassID(s));
                                                         ^~~~~~~~~~
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/lib/sanitizer_common/tests/../sanitizer_allocator_size_class_map.h:202:35: error: format specifies type 'size_t' (aka 'unsigned int') but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Werror,-Wformat]
    Printf("Total cached: %zu\n", total_cached);
                          ~~~     ^~~~~~~~~~~~
                          %lu
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/lib/sanitizer_common/tests/../sanitizer_allocator_size_class_map.h:198:11: error: format specifies type 'size_t' (aka 'unsigned int') but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Werror,-Wformat]
          i, Size(i), d, p, l, MaxCachedHint(s), cached, ClassID(s));
          ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp:190:10: note: in instantiation of member function '__sanitizer::SizeClassMap<2, 5, 9, 16, 64, 14>::Print' requested here
  SCMap::Print();
         ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp:203:3: note: in instantiation of function template specialization 'TestSizeClassMap<__sanitizer::SizeClassMap<2, 5, 9, 16, 64, 14>>' requested here
  TestSizeClassMap<VeryCompactSizeClassMap>();
  ^
Aug 16 2021, 1:09 PM
rupprecht updated subscribers of rG435756206700: [NFC][AArch64] Fix unused var in release build.
Aug 16 2021, 10:40 AM
rupprecht committed rG435756206700: [NFC][AArch64] Fix unused var in release build (authored by rupprecht).
[NFC][AArch64] Fix unused var in release build
Aug 16 2021, 10:08 AM

Jul 20 2021

rupprecht added a comment to D106226: [lldb] Improve error message when "lldb attach" fails.

Congrats on getting started on your first patch! I improving this error message really seems like a good idea.

From what I can see the error message here is identical to GDB's which is a different project with an incompatible license. No idea if this is large enough of a copy to bring us into the realm of copyright (not a lawyer), but I think formulating our own (maybe even better?) error message would anyway be a good idea. What about something along those lines:

error: attach failed: <Whatever error we already would return here> (This line is just the normal LLDB attach error)
Note that attaching might have failed due to the ptrace_scope security policy
which restricts debuggers from attaching to other processes. See
the ptrace_scope documentation for more information:
  https://www.kernel.org/doc/Documentation/security/Yama.txt
The current ptrace_scope policy can be found here:
  /proc/sys/kernel/yama/ptrace_scope

It'd also be helpful to include the actual command to enable it, i.e. either echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope or sudo sysctl -w kernel.yama.ptrace_scope=0 (I think both commands are equivalent)

Jul 20 2021, 1:43 PM · Restricted Project

Jul 15 2021

rupprecht added a comment to D100048: [lldb][Editline] Fix crash when navigating through empty command history..

lldb-arm-ubuntu (https://lab.llvm.org/buildbot/#/builders/17/builds/9015) fails with:

FAIL: LLDB (/home/tcwg-buildslave/worker/lldb-arm-ubuntu/build/bin/clang-arm) :: test_nav_arrow_up_empty_pr49845 (TestMultilineNavigation.TestCase)
======================================================================
ERROR: test_nav_arrow_up_empty_pr49845 (TestMultilineNavigation.TestCase)
   Tests that navigating with the up arrow doesn't crash.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tcwg-buildslave/worker/lldb-arm-ubuntu/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py", line 111, in expect_loop
    incoming = spawn.read_nonblocking(spawn.maxread, timeout)
  File "/home/tcwg-buildslave/worker/lldb-arm-ubuntu/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/pty_spawn.py", line 482, in read_nonblocking
    raise TIMEOUT('Timeout exceeded.')
pexpect.exceptions.TIMEOUT: Timeout exceeded.

The bot attributed this to my changes (https://reviews.llvm.org/D105629), but they have nothing to do with lldb or arm. I'm not sure whether it's a real issue or just a laggy builder, in which case increasing a timeout might be worth considering?

@rupprecht could you take a look please?

Jul 15 2021, 7:20 AM · Restricted Project

Jul 7 2021

rupprecht committed rG74c308c56a2d: [Bazel] Fixes for b5d847b1b95750d0af40cfc8c71a8fec50bb8613 and… (authored by rupprecht).
[Bazel] Fixes for b5d847b1b95750d0af40cfc8c71a8fec50bb8613 and…
Jul 7 2021, 4:53 PM

Jul 2 2021

rupprecht added a comment to D105275: [SLP]Fix gathering of the scalars by not ignoring UndefValues..

Thanks Alexey!

Jul 2 2021, 11:56 AM · Restricted Project

Jun 30 2021

rupprecht added a comment to D103458: [SLP]Improve gathering of scalar elements..

To me, the code is strange. You're allocating a variable on the stack, initialize it and then just exit from the function and the variable is not used anyware.

Jun 30 2021, 12:16 PM · Restricted Project

Jun 28 2021

rupprecht added a comment to D104261: Thread safety analysis: Always warn when dropping locks on back edges.

since it's restricted to relockable managed locks, I'm not too worried...

Not quite, it affects scoped locks with explicit unlock, which was supported before D49885.

@rupprecht, can you still test patches on Google's code? Would be good to know if this breaks anything.

Jun 28 2021, 7:31 PM · Restricted Project
rupprecht added a comment to D105055: [llvm-readobj] Make -s and -t match llvm-readelf.

Seems fine to me, but can you mention it in the release notes? Even though it's primarily a testing tool, downstream users may use llvm-readobj in their testing.

Jun 28 2021, 1:57 PM · Restricted Project

Jun 23 2021

rupprecht added a comment to D104791: Update Bazel BUILD files up to be9a87fe9b.

LGTM

Jun 23 2021, 5:27 PM · Restricted Project

Jun 22 2021

rupprecht added a comment to D102993: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls.

Sure I can take a look, but I don't see the immediate problem when looking at the backtrace.

I assume it takes some time to reduce the bug in Chrome? If you could get me the dump() output for the Decls D, (Decl*)ToDC, FromRD, ToD, (Decl*)ToD->getLexicalDeclContext() and whether the left or the right side of the && is true/false then maybe it becomes more obvious what's going wrong here.

This is probably not going to be useful:

  • D->dump(): FieldDecl 0x169993f9fbd0 <<invalid sloc>> <invalid sloc> __r_ 'std::__compressed_pair<std::basic_string<char, std::char_traits<char>, std::allocator<char> >::__rep, std::allocator<char> >'
  • ((Decl*)ToDC)->dump() - prints:
a decl that inherits DeclContext isn't handled
UNREACHABLE executed at llvm-project/clang/lib/AST/DeclBase.cpp:928!

ToDC->getDeclKindName() is "ClassTemplateSpecialization". Does that explain the unreachable error message? Maybe it needs a special case somewhere?

Jun 22 2021, 7:32 PM · Restricted Project
rupprecht added a comment to D103458: [SLP]Improve gathering of scalar elements..

We're seeing a test failure (and true miscompile AFAICT) in the CVC4 library that bisects to this patch. I don't have a nice reduction, but I can describe the issue we see.

In Constraint::externalExplain(AssertionOrder order), we construct a NodeBuilder [1]:

NodeBuilder<> nb(kind::AND);

That constructor expands to this [2]:

inline NodeBuilder(Kind k) :
  d_nv(&d_inlineNv),
  d_nm(NodeManager::currentNM()),
  d_nvMaxChildren(nchild_thresh) {
  Assert(k != kind::NULL_EXPR && k != kind::UNDEFINED_KIND)
      << "illegal Node-building kind";

  d_inlineNv.d_id = 1; // have a kind already
  d_inlineNv.d_rc = 0;
  d_inlineNv.d_kind = expr::NodeValue::kindToDKind(k);
  d_inlineNv.d_nchildren = 0;
}

d_inlineNv is a local class data member, and d_nv by default is just a pointer to that data member (but can be reassigned to point to something heap allocated)

The fields of d_inlineNv should be zero except for d_id which is 1, and d_kind which is 21 (corresponding to kind::AND). However after this commit, the struct is initialized with poison. The IR diff we see is this:

define dso_local void @_ZNK4CVC46theory5arith10Constraint15externalExplainEm(%"class.CVC4::NodeTemplate"* noalias sret(%"class.CVC4::NodeTemplate") align 8 %agg.result, %"class.CVC4::theory::arith::Constraint"* nonnull align 8 dereferenceable(144) %this, i64 %order) local_unnamed_addr #15 align 32 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
...
-  %40 = phi <2 x i64> [ %phi.bo220, %37 ], [ <i64 1, i64 21>, %if.else69 ]
+  %40 = phi <2 x i64> [ %phi.bo220, %37 ], [ poison, %if.else69 ]
...
  %42 = bitcast %"class.CVC4::NodeBuilder.623"* %nb to <2 x i64>*
  store <2 x i64> %40, <2 x i64>* %42, align 16

(%phi.bo220 is a path never taken AFAICT)

[1] https://github.com/CVC4/CVC4-archived/blob/40eac7f0529176bcc8464d6c4c8804fbde628c2b/src/theory/arith/constraint.cpp#L1563
[2] https://github.com/CVC4/CVC4-archived/blob/40eac7f0529176bcc8464d6c4c8804fbde628c2b/src/expr/node_builder.h#L377

Hi, really need a reproducer and a command line.

Understood -- this is just an initial report with some early analysis; I've occasionally gotten lucky in reporting bugs.

Jun 22 2021, 11:53 AM · Restricted Project
rupprecht added a comment to D103458: [SLP]Improve gathering of scalar elements..

We're seeing a test failure (and true miscompile AFAICT) in the CVC4 library that bisects to this patch. I don't have a nice reduction, but I can describe the issue we see.

Jun 22 2021, 10:35 AM · Restricted Project

Jun 21 2021

rupprecht committed rGb650778dc4ac: [NFC] Wrap entire assert-only block in LLVM_DEBUG (authored by rupprecht).
[NFC] Wrap entire assert-only block in LLVM_DEBUG
Jun 21 2021, 4:02 AM

Jun 17 2021

rupprecht added a comment to D101713: [OpaquePtr] Use ArgListEntry indirect types more in ISel lowering.

I don't see any messages in the reverts, but this change caused crashes both times it landed, in case that wasn't already the reason that it was reverted each time.

Jun 17 2021, 9:26 AM · Restricted Project
rupprecht added a comment to rG7647cb14dcd0: Revert "[NFC] Use ArgListEntry indirect types more in ISel lowering".

Some detail in the commit message of a revert explaining what issues motivated the revert would be handy

Jun 17 2021, 9:25 AM

Jun 15 2021

rupprecht committed rG6d33362dafb6: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}. (authored by rupprecht).
[libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}.
Jun 15 2021, 7:56 AM
rupprecht closed D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..
Jun 15 2021, 7:56 AM · Restricted Project

Jun 14 2021

rupprecht updated the diff for D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..
  • Require tsan feature to avoid buildbot jobs that don't support -fsanitize=thread (asan, windows, 32bit, ARMv7/8)
Jun 14 2021, 7:14 PM · Restricted Project
rupprecht added a comment to D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..

Thanks! I'll wait for another CI notification to post before landing.

Jun 14 2021, 4:13 PM · Restricted Project
rupprecht updated the diff for D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..
  • Add requires for clang to avoid unknown gcc flag errors
Jun 14 2021, 4:13 PM · Restricted Project
rupprecht added a comment to D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..

The test I added is not the greatest test, but it catches the issue. Lemme know if this works.

Jun 14 2021, 12:36 PM · Restricted Project
rupprecht updated the diff for D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..
  • Add a codegen test
Jun 14 2021, 12:33 PM · Restricted Project

Jun 10 2021

rupprecht added a comment to D102993: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls.

Sure I can take a look, but I don't see the immediate problem when looking at the backtrace.

I assume it takes some time to reduce the bug in Chrome? If you could get me the dump() output for the Decls D, (Decl*)ToDC, FromRD, ToD, (Decl*)ToD->getLexicalDeclContext() and whether the left or the right side of the && is true/false then maybe it becomes more obvious what's going wrong here.

Jun 10 2021, 4:39 PM · Restricted Project
rupprecht added a comment to D102993: [lldb] Disable minimal import mode for RecordDecls that back FieldDecls.

This commit seems to be causing an LLDB crash. I'm still working on gathering info and reducing it, but maybe the crash reason is obvious to you given this stack trace:

Jun 10 2021, 12:58 PM · Restricted Project

Jun 8 2021

rupprecht added a comment to D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..

Is it possible to write a test for that? When you say

This was caught by tsan tests after D99434 [...]

Do you mean that any Clang that includes the fix for D99434 will catch the error with the existing libc++ test suite when TSAN is enabled? If so, then I think it's fine not to add a test, since it's already tested. We could consider moving more CI configurations to Clang trunk, too.

Requesting changes until I have answers to my questions, but otherwise this LGTM.

Jun 8 2021, 11:13 AM · Restricted Project

Jun 7 2021

rupprecht requested review of D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}..
Jun 7 2021, 2:57 PM · Restricted Project

Jun 3 2021

rupprecht added a comment to D101191: [InstCombine] Fully disable select to and/or i1 folding.

The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.

Filed as http://llvm.org/PR50500

We needed SLP to get to the problem state in that example, but the bug was in instcombine/instsimplify.
Should be fixed with:
7bb8bfa0622b

Jun 3 2021, 12:37 PM · Restricted Project, Restricted Project

May 27 2021

rupprecht added a comment to D100651: [AIX] Support of Big archive (read).

While building this, there's a warning about an unhandled switch:

It occurs on ArchiveWriter.cpp. I implement a way to write Big Archive. If you think it is better, I can add some code to remove warning, but my goal is to implement read and write for Big Archive, so it will be corrected in a future commit / PR.

Many people build with -Werror, so it would be good to handle the case even if it's explicitly ignored, like:

May 27 2021, 5:03 PM · Restricted Project
rupprecht committed rGe41aaea26238: [NFC][libObject] clang-format Archive{.h,.cpp} (authored by rupprecht).
[NFC][libObject] clang-format Archive{.h,.cpp}
May 27 2021, 4:51 PM

May 26 2021

rupprecht added a comment to D101191: [InstCombine] Fully disable select to and/or i1 folding.

The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.

May 26 2021, 4:46 PM · Restricted Project, Restricted Project
rupprecht added a comment to D101191: [InstCombine] Fully disable select to and/or i1 folding.

The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.

May 26 2021, 12:13 PM · Restricted Project, Restricted Project
rupprecht added a comment to D101191: [InstCombine] Fully disable select to and/or i1 folding.

FYI, I'm seeing what I think is a miscompile that bisects to this patch. Greatly simplified, the problematic snippet is this:

May 26 2021, 9:51 AM · Restricted Project, Restricted Project
rupprecht added a comment to D103164: [SLP]Fix vectorization of insertelements with mutiple uses..

Thanks, this fixes the miscompile I'm seeing.

May 26 2021, 8:02 AM · Restricted Project

May 25 2021

rupprecht added a comment to D101555: [SLP]Improve handling of compensate external uses cost..

We're seeing some test failures that bisected to this patch, possibly a miscompile. The test failure is in the unit test for this file: https://github.com/google/tink/blob/master/cc/subtle/aes_eax_aesni.cc. Are there already any known issues with this patch?

No, there are not. It would help if you could provide the reproducer and exact compile command to check if the problem exists.

May 25 2021, 7:16 PM · Restricted Project
rupprecht added a comment to D101555: [SLP]Improve handling of compensate external uses cost..

We're seeing some test failures that bisected to this patch, possibly a miscompile. The test failure is in the unit test for this file: https://github.com/google/tink/blob/master/cc/subtle/aes_eax_aesni.cc. Are there already any known issues with this patch?

May 25 2021, 11:05 AM · Restricted Project

May 13 2021

rupprecht added a comment to D98714: [SLP] Add insertelement instructions to vectorizable tree.

This patch introduces an assertion error we believe may be contributing to a miscompile (along with some other recent SLP patches -- this patch fixes the reduced case in http://llvm.org/PR50323, but doesn't fix the full case it was reduced from):

May 13 2021, 7:46 PM · Restricted Project

May 12 2021

rupprecht added a comment to D95543: [GVN] Clobber partially aliased loads..

Updated patch LG -- the unreduced test passes now (and has no assertion errors). Thanks! (Deferring to @nikic or others for actual re-review).

May 12 2021, 8:45 AM · Restricted Project
rupprecht committed rG1336c5ae2fea: [llvm-cov][test] Add test coverage for "gcov" implying "llvm-cov gcov"… (authored by rupprecht).
[llvm-cov][test] Add test coverage for "gcov" implying "llvm-cov gcov"…
May 12 2021, 8:25 AM
rupprecht closed D102299: [llvm-cov][test] Add test coverage for "gcov" implying "llvm-cov gcov" compatibility..
May 12 2021, 8:25 AM · Restricted Project
rupprecht added a comment to D102299: [llvm-cov][test] Add test coverage for "gcov" implying "llvm-cov gcov" compatibility..

Is it strictly ends with, or do things like file extensions, version numbers etc have an impact? See the additional test cases in https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-objcopy/tool-name.test.

May 12 2021, 8:21 AM · Restricted Project

May 11 2021

rupprecht requested review of D102299: [llvm-cov][test] Add test coverage for "gcov" implying "llvm-cov gcov" compatibility..
May 11 2021, 6:39 PM · Restricted Project
rupprecht added a comment to D95543: [GVN] Clobber partially aliased loads..

Temporarily reverted in fec2945998947f04d672e9c5f33b57f7177474c0 to keep trunk clean.

May 11 2021, 5:50 PM · Restricted Project
rupprecht added a reverting change for rG6c570442318e: [GVN] Clobber partially aliased loads.: rGfec294599894: Revert "[GVN] Clobber partially aliased loads.".
May 11 2021, 4:09 PM
rupprecht committed rGfec294599894: Revert "[GVN] Clobber partially aliased loads." (authored by rupprecht).
Revert "[GVN] Clobber partially aliased loads."
May 11 2021, 4:09 PM