Thanks! This indeed fixes the issue I'm seeing.

This seems to cause a regression on Linux where we no longer get the signal details.

In D146590#4211763, @bulbazord wrote:

lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"

Just tried this, didn't crash LLDB but it does give me the OverflowError you mentioned.

Can you add a regression test that invokes lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"? Just a simple shell test should suffice.

Seems like most xml-based tests like this are in lldb/test/API/functionalities/gdb_remote_client, is there a reason this is not in that package too?

In D144958#4207153, @ABataev wrote:

In D144958#4206872, @aeubanks wrote:

a more succinct version: https://godbolt.org/z/7YTqP89Mv shows that slp-vectorizer introduces loads from poison

Must be fixed in 59ff9d3777701ebbe6a59ab2edb8792ef3d2873f

We're still seeing issues with this at trunk, even after the followup assertion fix in 874c49f55454cb285282e6d184f809945c0beca1.

In D144285#4163004, @cor3ntin wrote:

If however we find this change to disruptive, we should inform WG21.

5195e14bc1222e1b3f1b7b43c04e1c1ac3504cb1 just landed the same thing as this, although it didn't put it in the sorted order. Feel free to land this if you remove the other entry.

Thanks! This seems to fix all the errors I was seeing before.

see: D145129

Thanks!

I think the issue is:

+      // Byte shifts can be slower so only match them on second attempt.
+      if (Order == 0 &&
+          (Shuffle == X86ISD::VSHLDQ || Shuffle == X86ISD::VSRLDQ))
+        continue;

It comes before the check of

+      if (0 < ShiftAmt && (!ShuffleVT.is512BitVector() || Subtarget.hasBWI() ||
+                           32 <= ShuffleVT.getScalarSizeInBits())) {
+        PermuteImm = (unsigned)ShiftAmt;
+        return true;
+      }

and the 0 < ShiftAmt check if basically a check if actually found/set Shuffle.
Don't think the bug actually can change behavior but is bug none the less.

Will post patch to fix.

After this patch, I see an msan issue running this test internally; strangely I don't see a failure on any sanitizer buildbot yet.

Here's one change this patch causes on "real" code (invalid code, but something a user might try to compile): we see is a static_assert in gmock that now fails to report a useful error message: https://godbolt.org/z/sPr1PYT9d

• Fix comment typo

This has already been reverted, but I found a breakage (not a crash) caused by this:

#include <type_traits>

All the things that were broken before are no longer broken, so LGTM

The total impact for us is ~400 files, but very trivial in most cases, and usually only a couple impacted lines per file.

In D143914#4127330, @ldionne wrote:

In D143914#4125054, @rupprecht wrote:

The only other breakage I found so far that might be worthy of mention is that constructing containers requires begin/end iterators of pairs, not tuples, but frameworks for doing functional-style programming often tuples to be fully generic, so this breaks. This seems like an intended effect of this patch, but makes things a little less ergonomic. Is there an idiomatic way to do that w/o having to write your own tuple->pair boilerplate?

Switching to -std=c++2b would fix that -- IMO that sounds reasonable but I am interested to know if you disagree.

It isn't an option for us, but I don't think it's worth it to keep a non-conformant extension + complexity in libc++ just so we can avoid a little boilerplate to do tuple->pair manually in a couple places.

The only other breakage I found so far that might be worthy of mention is that constructing containers requires begin/end iterators of pairs, not tuples, but frameworks for doing functional-style programming often tuples to be fully generic, so this breaks. This seems like an intended effect of this patch, but makes things a little less ergonomic. Is there an idiomatic way to do that w/o having to write your own tuple->pair boilerplate?

Still chugging away, looks like this is going to be at least 50 breakages. Most of them are trivial to fix (e.g. return std::make_tuple() in a method returning std::pair should be return std::make_pair()). There are a few that are a little surprising, such as this one which doesn't seem related to tuple compatibility:

In D143914#4123629, @ldionne wrote:

In D143914#4123385, @rupprecht wrote:

In D143914#4123012, @ldionne wrote:

Pinging vendors since this is technically a source break. I'll also take this for a run internally to see how much breakage this causes, if any.

This one is a bit bigger of a breakage for us. Do you have a target date for landing this patch?

I'd like to do it before the LLVM 17 release, and ideally not too late in the cycle to allow everyone to see this breakage well in advance. How bad is it?

In D143914#4123012, @ldionne wrote:

Pinging vendors since this is technically a source break. I'll also take this for a run internally to see how much breakage this causes, if any.

In D143346#4113858, @ldionne wrote:

Thanks for fixing this! Only nits.

Let's land this and cherry-pick it onto LLVM 16. Please add a release note on the LLVM 16 release branch to explain that this constrains std::shared_ptr more than it used to, which could lead to a bit of source break. I don't think this should be too big of a deal, especially since the code this would break is arguably brittle.

Pinging vendors for awareness.

In D143678#4116440, @gchatelet wrote:

I just looked at the premerge checks and I can see how this will be hard to fix globally (if at all possible).
I'll use the package feature instead.

In D124351#4102679, @aaron.ballman wrote:

In D124351#4102634, @eaeltsin wrote:

Looks like we fail to enter the appropriate context somewhere (my guess is that it might be specific to the attribute but it's hard to say without picking around), definitely a bug

I'll be away the next 2 weeks, I'll look at it when I get back. Maybe we should track it in an issue though.

Should we revert this then?

Yes, I think we should. We should also file an issue so @cor3ntin doesn't lose track of the issue. Anyone want to volunteer to do that? (I can do it early next week otherwise.)

Hi, me again :)

In D133661#4096429, @alexfh wrote:

In D133661#4096263, @alexfh wrote:

@philnik could you commit one of the proposed abi_tag fixes?

Actually, I went ahead and committed it as 561105fb9d3a16f7fb8c718cc5da71b11f17a144 to unblock us. Hopefully, that's small and obvious enough to not violate the code review policies.

The ABI tag approach fixes the ODR issue I was looking at

Not a breakage report, but just a comment in case it wasn't expected/intended: it looks like this increases the conformance requirement for custom iterators passed to std::copy and friends. From a couple tests, it seems like gcc/libstdc++ already has stricter requirements than libc++, so the conformance requirement is being matched between libc++ and libstdc++ now. As an example:

Thanks!

I think I see why we don't see this failure internally (maybe), but I'm clueless as to why the buildkite builder or local builds don't complain. Anyway, thanks!

LGTM

This looks like a nice improvement! Just one thing I noticed.

• Remove redundant comment

In D141175#4038017, @GMNGeoffrey wrote:

It seems like the same logic would extend to the CMake build. Could we make the same change there?

• Move annotations to a separate package entirely

In D136554#4024628, @rupprecht wrote:

I still see one behavior change (actually it was there before, but I missed it in the test results), but as far as I can tell, it's a good one? If I reduce it too much, I get the warning with the baseline toolchain, so it's not erroneous AFAICT. Although I won't pretend I know all the intricacies of static and inline.

I still see one behavior change (actually it was there before, but I missed it in the test results), but as far as I can tell, it's a good one? If I reduce it too much, I get the warning with the baseline toolchain, so it's not erroneous AFAICT. Although I won't pretend I know all the intricacies of static and inline.

// a.h
static const std::pair<double, double>& GetFakePair() {
  static constexpr std::pair<double, double> kFakePair = {123.0, 456.0};
  return kFakePair;
}

If you want to keep this test but just not run it by default, can you put it in a separate target that the build bot doesn't run? I guess it's up to you.

I threw this at the "test everything" test (some millions of targets) and it found only one breakage, so this is very very close. Without further ado, here is this silly looking thing:

I'm not sure what to make of the new failure when I try it out this time. Given a source like this:

#include <functional>

In D136554#4018870, @rupprecht wrote:

All good now! The latest revision of this patch doesn't seem to break anything, unless I ran our tests wrong. From my perspective this is OK to reland now.

All good now! The latest revision of this patch doesn't seem to break anything, unless I ran our tests wrong. From my perspective this is OK to reland now.

In D136554#4013463, @rupprecht wrote:

Glad the test case made sense to you, it was convoluted to me :)

Still seeing one more error, and it's not modules-related so I might be able to get it reduced today. Generally, it looks like this:

struct Inner {
  Foo& foo;
  const std::unique_ptr<...> x = blah(blah(
      &foo.bar()));
};

class Outer {
 private:
  Foo foo_;
  Inner inner{foo_};
}

With the error being:

error: 'Inner::foo' is not a member of class 'Outer'
      &foo.bar()));

I think this build failure is wrong? foo should be referring to the definition inside Inner, but clang seems to be expecting it to refer to something in Outer.

Is it expected that this patch will cause some previously "working" code to no longer build? At some point I expect to hand you a reduction that's actually a bug in the user code.

Glad the test case made sense to you, it was convoluted to me :)

ping :)

Sorry for the delay, I was out on vacation for a bit. I have a repro for this new issue now:

In D139741#4000201, @rupprecht wrote:

I'm not sure what the libcxx failure was that caused you to revert this, but we also saw a clang crasher as a result of this. clang/lib/AST/Decl.cpp:4300 in unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: isBitField() && "not a bitfield". I'll try to reduce it.

I applied this version of the patch and the crash is now gone 🎉

In D140058#4000311, @rupprecht wrote:

This is causing a test failure: https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
(and likewise running ninja check-clang-unit locally, but I don't have a buildbot link to that)

This is causing a test failure: https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
(and likewise running ninja check-clang-unit locally, but I don't have a buildbot link to that)

I'm not sure what the libcxx failure was that caused you to revert this, but we also saw a clang crasher as a result of this. clang/lib/AST/Decl.cpp:4300 in unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: isBitField() && "not a bitfield". I'll try to reduce it.

Here's a well-formed reproducer:

Actually, that assertion failure is pre-existing. However, this is newly failing in a no-asserts clang, so I wonder if something about this patch is just surfacing an existing bug in clang. Anyway, I hope to have a better repro by EOD.

Ugh, I left cvise running overnight and forgot to include the validity check by building with a previous clang, so my reduction is invalid. I'm going to run it again, but here's the invalid crasher in the meantime:

Looks like the latest reland still has some issue remaining. With asserts enabled, I get: assert.h assertion failed at clang/include/clang/AST/Type.h:752 in const ExtQualsTypeCommonBase *clang::QualType::getCommonPtr() const: !isNull() && "Cannot retrieve a NULL type pointer". I'm going to work on reducing it now.