In D118950#3297998, @ldionne wrote:

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.

In D118940#3295359, @philnik wrote:

In D118940#3295347, @Quuxplusone wrote:

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.

In D118940#3295112, @Quuxplusone wrote:

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.

• Restoring original version of the patch

• Undo clang-formatting

• Use _LIBCPP_HIDE_FROM_ABI
• Use `_LIBCPP_DEBUG_LEVEL
• Fix clang-format CI test failure
• Fix whitespace in test

SGTM

In D118053#3266716, @sivachandra wrote:

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

Rebase

In D113449#3121070, @teemperor wrote:

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.

• Use better test APIs, add test back w/ a @expectedFailure

• Switch crash repro from shell -> api test

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.

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

In D110867#3106660, @MatzeB wrote:

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?

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

In D108389#3015689, @ldionne wrote:

In D108389#2973427, @jyknight wrote:

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

In D108389#2973423, @rupprecht wrote:

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

In D108389#2965032, @ldionne wrote:

In D108389#2963744, @jyknight wrote:

In D108389#2955525, @ldionne wrote:

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?

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.

In rG45138f788c9b3c4ac5d9ae4479841c411c15190e#1020200, @fhahn wrote:

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>();
  ^

In D106226#2891107, @teemperor wrote:

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)

In D100048#2879669, @iii wrote:

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?

Thanks Alexey!

In D103458#2851005, @ABataev wrote:

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.

In D104261#2844636, @aaronpuchert wrote:

In D104261#2841356, @delesley wrote:

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.

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.

LGTM

In D102993#2811902, @rupprecht wrote:

In D102993#2811337, @teemperor wrote:

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?

In D103458#2833760, @ABataev wrote:

In D103458#2833713, @rupprecht wrote:

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.

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.

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.

In rG7647cb14dcd0c471d1bc53b6a864c4737e0e82e8#997632, @dblaikie wrote:

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

• Require tsan feature to avoid buildbot jobs that don't support -fsanitize=thread (asan, windows, 32bit, ARMv7/8)

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

• Add requires for clang to avoid unknown gcc flag errors

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

• Add a codegen test

In D102993#2811337, @teemperor wrote:

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

In D103846#2805849, @ldionne wrote:

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.

In D101191#2788596, @spatel wrote:

In D101191#2783570, @rupprecht wrote:

In D101191#2782963, @rupprecht wrote:

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

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:

In D101191#2782963, @rupprecht wrote:

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.

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.

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

Thanks, this fixes the miscompile I'm seeing.

In D101555#2780152, @ABataev wrote:

In D101555#2780145, @rupprecht wrote:

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.

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?

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

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

In D102299#2753300, @jhenderson wrote:

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.

Temporarily reverted in fec2945998947f04d672e9c5f33b57f7177474c0 to keep trunk clean.