In D158942#4626240, @rnk wrote:

+@rupprecht @aeubanks, can you review this Bazel change?

In D153156#4600270, @rupprecht wrote:

of which there's an example in LLVM itself: https://github.com/llvm/llvm-project/blob/6a0e536ccfef1f7bd64ee4153b4efc0aeecf28d4/clang/test/SemaCXX/cxx11-compat.cpp#L38

Sorry, I don't want to mislead: I just mean there's a handy example in clang's unit tests. I don't see any instances of this in LLVM non-test code.

The other common breakage I'm seeing is code that hasn't yet migrated from the "PRI" format macros, of which there's an example in LLVM itself: https://github.com/llvm/llvm-project/blob/6a0e536ccfef1f7bd64ee4153b4efc0aeecf28d4/clang/test/SemaCXX/cxx11-compat.cpp#L38

In D153156#4599106, @aaron.ballman wrote:

In D153156#4598988, @rZhBoYao wrote:

In D153156#4598915, @steelannelida wrote:

Unfortunately the option -Wno-reserved-user-defined-literal fails after this:

#define MYTHING "_something_"

const char* f() {
  return "ONE"MYTHING"TWO";
}

$ clang -Wno-reserved-user-defined-literal repro.cxx
repro.cxx:4:15: error: no matching literal operator for call to 'operator""MYTHING' with arguments of types 'const char *' and 'unsigned long', and no matching literal operator template
    4 |   return "ONE"MYTHING"TWO";
      |               ^
1 error generated.

This is conforming right? Correct me if I'm wrong. My reading of https://eel.is/c++draft/lex.pptoken#3.3 is that "ONE"MYTHING"TWO" is a single preprocessing-token during phase 3 (https://eel.is/c++draft/lex.phases#1.3). Can @aaron.ballman confirm this?

The diagnostic behavior is correct. MYTHING doesn't get expanded until phase 4 (http://eel.is/c++draft/lex.phases#1.4), so this appears as "ONE"MYTHING as a single preprocessor token: https://eel.is/c++draft/lex.ext#nt:user-defined-string-literal and that token is an invalid UDL.

Reduced:

struct a {
  a(decltype(nullptr));
  long b;
} *c;
enum { d };
struct t {
  _Atomic(a) e;
};
struct f : t {};
struct atomic {
  f g;
  void h() {
    f *j = &g;
    t *k = j;
    __c11_atomic_load(&k->e, d);
  }
};
a l(a *addr) {
  reinterpret_cast<atomic *>(addr)->h();
  return 0;
}
struct {
  void m(long v) {
    long w = sizeof(this);
    c = reinterpret_cast<a *>(w);
    a *o = c;
    l(&o[v]);
  }
} p;
a q();
long r;
bool u();
a s() {
  if (u()) return q();
  for (long i = r; i; i++) p.m(i);
  return nullptr;
}

fyi, I bisected a crash during codegen prepare to this commit. I'm trying to reduce a repro now.

LGTM, although this logic is already pretty obscure, so it's hard to follow. I wonder if we have fuzzers hooked up to this so we don't have to worry too much if this misses an edge case.

In D154014#4457883, @MaskRay wrote:

This is a breaking change since some SCLs might use .* or (abc|def) which are supported regexes but not valid globs. Since we have just cut clang 16.x this is a good time to make this change.

My user has some ignore lists, but there is no ^[a-z]+:.*\( or ^[a-z]+:\. occurrence, so this change is likely safe for us.

In D150226#4401188, @jyknight wrote:

In D150226#4400782, @rupprecht wrote:

As a general question/feature request: is there a way to have specific warnings apply even for system headers? It would be nice if I could check what breaks when by adding -Wsystem-error=enum-constexpr-conversion to the global build flags. Rebuilding clang w/ this patched in also works, but is a little more difficult/noisy.

I think there's no per-warning flag (you can turn on _all_ of the enabled warnings in system headers with -Wsystem-headers), but by modifying the clang sources you can add ShowInSystemHeader to the diagnostic.
E.g.

 def warn_constexpr_unscoped_enum_out_of_range : Warning<
   "integer value %0 is outside the valid range of values [%1, %2] for this "
-  "enumeration type">, DefaultError, InGroup<DiagGroup<"enum-constexpr-conversion">>;
+  "enumeration type">, DefaultError, InGroup<DiagGroup<"enum-constexpr-conversion">>, ShowInSystemHeader;

In D150226#4353907, @aaron.ballman wrote:

In D150226#4353863, @jyknight wrote:

When looking for errors in existing codebases, don't forget that this diagnostic is currently suppressed by default in system headers. So this patch is moving from "no diagnostics for code in system headers" to "unconditional hard error in system headers". Just removing -Wno-enum-constexpr-conversion from your build flags is insufficient to test that this patch won't break code!

I haven't done any tests, but I'm rather skeptical that this change is going to be viable without breaking stuff, still.

+1 to needing to test against system headers, but also: if we find any system headers that would be broken by this, we should proactively alert the owners of those headers so that they understand there's urgency to getting the fixes into their headers so that the entire ecosystem isn't held back. Alternatively, if it's just one problematic system header in an LTS release somewhere, we could perhaps put in a compat hack for just that header so we can move forward.

The other tricky part I missed before is this bit in between pulling the event off the listener queue and deciding to show it:

In D150805#4350849, @JDevlieghere wrote:

I also like Jordan's rate limiting idea. In my mind that should be a property of the broadcaster. Different tools (e.g. vscode vs the command line) could specify different values when register a listener.

In D150322#4356560, @Mordante wrote:

In D150322#4354896, @rupprecht wrote:

I forgot about this patch but I won't be around to watch it go through buildbots etc., so I'll plan to land it as soon as I get back in a couple weeks.

Does this main the breakage of D150015 will remain for a couple of weeks. If so I think we should revert D150015 and look at a good solution for you when you're back. WDYT?

I forgot about this patch but I won't be around to watch it go through buildbots etc., so I'll plan to land it as soon as I get back in a couple weeks.

I found one use here: https://github.com/ericniebler/range-v3/blob/48dc2eb666c07e6afc8ec5edf7db9a5c6cde7a56/include/range/v3/functional/invoke.hpp#L51

This is a nice proof of concept, but I think we should go with a time-based approach to rate limit this. (Anyone else in LLDB know if we have some utils around to help with this?)

In D150226#4338264, @shafik wrote:

In D150226#4336755, @rupprecht wrote:

We're still using -Wno-enum-constexpr-conversion, although I'm not sure if we need that or if we just forgot to remove it after doing some cleanup. I'm trying it out now. (Sorry, I'm not sure we were aware that having a way to turn this off was just temporary).

Please let me know how things look on your end.

In D150639#4346649, @JDevlieghere wrote:

In D150639#4346009, @rupprecht wrote:

+1 to being surprised this is not already the case

Some other places should be updated after this, e.g. lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially updated:

#if defined(LLDB_CONFIGURATION_DEBUG)
  // We assert that we have to module lock by trying to acquire the lock from a
  // different thread. Note that we must abort if the result is true to
  // guarantee correctness.
  assert(std::async(
             std::launch::async,
             [this] {
               return this->GetModuleMutex().try_lock();
             }).get() == false &&
         "Module is not locked");
#endif

-> just wrap it in lldbassert

This code actually has a comment right above it explaining that it's too expensive to do in release builds, so this isn't a good candidate for lldbassert. But this logic can be improved: it duplicates checking the LLDB_CONFIGURATION_DEBUG in both ASSERT_MODULE_LOCK and AssertModuleLock and should use NDEBUG too. I'll address that in a separate patch.

WDYT about just replacing LLDB_CONFIGURATION_DEBUG with LLVM's EXPENSIVE_CHECKS in this case?

+1 to being surprised this is not already the case

In D150068#4336864, @skan wrote:

In D150068#4336844, @rupprecht wrote:

Is it expected that this patch would cause any differences in the final binary? It seems to be just a refactoring, but we bisected a runtime crash to this commit.

It should not cause any difference in binary in theory. Could you show the difference or provide a small reproducer?

Is it expected that this patch would cause any differences in the final binary? It seems to be just a refactoring, but we bisected a runtime crash to this commit.

We're still using -Wno-enum-constexpr-conversion, although I'm not sure if we need that or if we just forgot to remove it after doing some cleanup. I'm trying it out now. (Sorry, I'm not sure we were aware that having a way to turn this off was just temporary).

Is there a test case you can add for this?

• Fix some indentation

• Add a comment explaining __CORRECT_ISO_CPP_WCHAR_H_PROTO, and uncomment the actual fix now that I'm done testing CI

Sadly, when I commented out the fix & ran it through CI, the only failure was on the "no wide characters" bot because I didn't have the right XFAIL line for that. I guess we don't have any bots with an old enough glibc?

• Add XFAIL: no-wide-characters to fix CI, and update the comment with bug details

• Add some tests, and temporarily comment out the fix to see if CI catches it.

In D150015#4332022, @ldionne wrote:

Does any existing test fail when we run the test suite with glibc-2.25, or do we need to add a test case for this?

No existing test case that I'm aware of. Let me see if the repro Ray posted can be added to the test suite.

Restore to v1 of the patch

No non-determinism detected with this on top of D150210, so LGTM. Thanks!

In D150015#4324668, @philnik wrote:

IMO we should just #include <wchar.h> and <uchar.h> instead. That way we don't have to do anything funky in multiple headers. In fact, our wchar.h fixes up some interfaces for non-C++-friendly C libraries.

• Use #include instead of setting __CORRECT_ISO_CPP_WCHAR_H_PROTO again

In D146622#4330046, @rupprecht wrote:

This is causing a compiler crash, which looks like a div by zero. I'll start working on a repro, but I'd like to revert to keep trunk green.

Reverted in e08c397a88077c50dc25e71b39b9d5efbfc85a9a

This is causing a compiler crash, which looks like a div by zero. I'll start working on a repro, but I'd like to revert to keep trunk green.

In D150015#4324668, @philnik wrote:

IMO we should just #include <wchar.h> and <uchar.h> instead. That way we don't have to do anything funky in multiple headers. In fact, our wchar.h fixes up some interfaces for non-C++-friendly C libraries.

I can't reproduce this outside of a modules-enabled build, and unfortunately I don't know how to write a modules test case. Anyway, here's the stderr I see:

wchar.h:316:29: error: functions that differ only in their return type cannot be overloaded
extern "C++" const wchar_t *wmemchr (const wchar_t *__s, wchar_t __c,
                   ~~~~~~~~~^
wchar.h:320:17: note: previous declaration is here
extern wchar_t *wmemchr (const wchar_t *__s, wchar_t __c, size_t __n)
       ~~~~~~~~~^

In D143897#4310910, @efriedma wrote:

All the operations involved here should be deterministic. You're probably looking for a bug elsewhere. One possibility is that LoopInfo doesn't contain the correct information... for example, the CFG changes, but that change isn't recorded in LoopInfo.

I'm not making very good progress with a test case. So far the only interesting insights I may have:

• This only reproduces in optimized builds. Setting -UNDEBUG makes the problem go away. Maybe debug builds are normalizing something?
• The problem obvious goes away with -mllvm -cgp-max-loop-inv-users-to-scan=0. One of my guesses was that V->uses() ordering might be non-deterministic, so you could get different answers in isUsedInLoop if the loop is terminated before you visit the instruction that would cause you to return true. That is not the case: the problem still exists with -mllvm -cgp-max-loop-inv-users-to-scan=10000.

Otherwise, if I do the usual process of getting clang to dump the IR and pipe that into opt, or various things like that, the process is deterministic. If you have any guess for what might get a reproducer, I can give it a shot.

@chill, are you able to reproduce this?

In D143897#4301532, @chapuni wrote:

This discovers unstable behavior.
For me, MCA/ResourceManaget.o sometimes differs.
Reproduced by stage1 clang.

In D143914#4292303, @ldionne wrote:

@rupprecht I'd like to land this within the next few days. If you're able to confirm that this doesn't cause as many issues for you anymore, that would be great. Otherwise, i'll land this without your confirmation -- I am fairly confident this new patch should solve the vast majority of issues you were seeing.

In D148862#4285702, @aaronmondal wrote:

@rupprecht Reran. Seems to work.

SGTM, I'm not sure if LLVM has any policy w/ bazel versions like it does for host toolchains or CMake. (Probably not, since bazel is not an officially supported build system for LLVM). As long as CI passes, this should be fine. Any specific reason you're bumping it?

Trying to get an actual reduction to share in case this isn't the right approach. The source is basically this:

In D146987#4263048, @MaskRay wrote:

The latest reland 3820e9a2b29a2e268319ed6635da0d59e18d736d still caused some issues to our internal workloads. Stay tuned...

So far we have only found crashes with ThinLTO workloads.

Thanks!

The pid plumbing looks fine for the happy case, but I think we could be more lenient if (for whatever reason) the pid flag isn't being set on non-Linux systems that won't actually be using it. Even on a Linux system, this is only needed if ptrace_scope != 0, so if the pid isn't being provided there, we could just skip the prctl call and hope the user's system has a value of 0 for that.

Note: this uses @platforms//os:android and not @bazel_tools//src/conditions:android because android is not one of the configs listed in @bazel_tools//src/conditions.

Test are passing again after this patch, so I think we're all good now. Thanks!

This relanded in e4ceb5a7bb9b8f6d730530345649286370dd3ff8, and we're seeing runtime errors in Halide when building with asan.

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.