ilya-biryukov added a comment. This LG, but not accepting as I believe libc++ is still broken if built with modules, right? I have run `check-cxx` locally and it seems to work, but I suspect that's not using modules by default. I have clicked through the links in phrabricator and the errors seem to come from the latest uploaded diff. Let me know if I'm missing something.
================ Comment at: clang/lib/Sema/SemaInit.cpp:5380 + } + InitExprs.push_back(ER.get()); + } ---------------- ayzhao wrote: > ilya-biryukov wrote: > > ayzhao wrote: > > > ayzhao wrote: > > > > So the libc++ test compile failures are due to this line. > > > > > > > > One example of a failing unit test is > > > > [range.take.while/ctor.view.pass](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp). > > > > Clang calls this function twice in `TreeTransform.h` - once with > > > > `VerifyOnly` set to `true`, once with it set to `false`. > > > > > > > > For some reason, when this function tries to value-initialize the > > > > member `MoveOnly mo` in `View`, `Seq.Failed()` returns false after > > > > `TryValueInitialization(...)`, but the resulting `ExprResult` is > > > > `nullptr`, causing the segfault we see when we push `nullptr` to > > > > `InitExprs` and pass `InitExprs` to the constructor of > > > > `CXXParenListInitExpr`. One way to be fix this is to move the line > > > > `ExprResult ER = Seq.Perform(...)` out of the `if (!VerifyOnly)` block > > > > and check for `ER.isInvalid()` instead of `Seq.Failed()`, but that > > > > results in test failures due to excess diagnostic messages in > > > > `Seq.Perform(...)` > > > > > > > > I'm still looking into this, but if anyone has any ideas, they would be > > > > very welcome. > > > > > > > > To repro the buildbot failures, just build clang with this patch, and > > > > then in a separate build directory, build the target `check-cxx` using > > > > the previously built clang. > > > I was able to get the above workaround to pass the test by clearing the > > > diagnostics after calling `Seq.Perform(...)`. > > > > > > IMO, this should be OK for now, but I'm open to better ideas if anyone > > > has any. > > Clearing all the diagnostics is a nuclear options and definitely seems off > > here. We should not call `Perform()` when `VerifyOnly` is `true` to avoid > > producing the diagnostics in the first place. > > > > It's fine for the call with `VerifyOnly = true` to return no errors and > > later produce diagnostics with `VerifyOnly = false`, I believe this is what > > `InitListChecker` is already doing. > > I have been playing around with the old version of the code, but couldn't > > fix it fully. I do have a small example that breaks, we should add it to > > the test and it should also be easier to understand what's going on: > > > > ``` > > struct MoveOnly > > { > > MoveOnly(int data = 1); > > MoveOnly(const MoveOnly&) = delete; > > MoveOnly(MoveOnly&&) = default; > > }; > > > > struct View { > > int a; > > MoveOnly mo; > > }; > > > > void test() { > > View{0}; > > View(0); // should work, but crashes and produces invalid diagnostics. > > } > > ``` > > > > In general, my approach would be to try mimicing what `InitListChecker` is > > doing as much as possible, trimming all the unnecessary complexity that > > braced-init-lists entail. > > Hope it's helpful. > So it looks like all I had to do was remove the call to > `TryValueInitialization(...)` and just check for `Seq.Failed()`. This is also > what we do in `InitListChecker`. > > The `check-cxx` target appears to work for me locally, so fingers crossed > that the build passes. > > > I do have a small example that breaks, we should add it to the test and it > > should also be easier to understand what's going on: > > Done. Good catch! I have also noticed that `TryValueInitialization` is missing in `InitListChecker`, but somehow thought it's just doing the same thing without reusing the code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129531/new/ https://reviews.llvm.org/D129531 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
