This is an archive of the discontinued LLVM Phabricator instance.

Implement CWG2631
ClosedPublic

Authored by cor3ntin on Oct 23 2022, 8:49 AM.

Details

Summary

Implement https://cplusplus.github.io/CWG/issues/2631.html.

Immediate calls in default arguments and defaults members
are not evaluated.

Instead, we evaluate them when constructing a
CXXDefaultArgExpr/BuildCXXDefaultInitExpr.

The immediate calls are executed by doing a
transform on the initializing expression.

Note that lambdas are not considering subexpressions so
we do not need to transform them.

As a result of this patch, unused default member
initializers are not considered odr-used, and
errors about members binding to local variables
in an outer scope only surface at the point
where a constructor is defined.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cor3ntin added inline comments.Nov 3 2022, 12:39 PM
clang/include/clang/Sema/Sema.h
9665–9667

I added parentheses here to make that clearer!

clang/lib/Sema/SemaExpr.cpp
5922

@shafk

clang/test/SemaCXX/cxx2a-consteval-default-params.cpp
11

I added a comment

cor3ntin marked an inline comment as done and an inline comment as not done.Nov 3 2022, 12:40 PM
cor3ntin added inline comments.
clang/lib/Sema/SemaExpr.cpp
5922

Oups, nvm

This revision was automatically updated to reflect the committed changes.
cor3ntin marked an inline comment as not done.

As discussed with Shafik and Aaron, I'm landing that now so there is some buffer to handle unforeseen issues as i won't be available next week and the one after. Further comments are of course welcomed.
If we don't find any issues we can land https://reviews.llvm.org/D120634 in the coming weeks

cor3ntin reopened this revision.Nov 4 2022, 12:24 AM
This revision is now accepted and ready to land.Nov 4 2022, 12:24 AM
cor3ntin updated this revision to Diff 473177.Nov 4 2022, 2:44 AM

I pushed a tentative fix for the bot failure,
alas I'm completely unable to reproduce the issue locally, even
though it does not appear to be architecture dependant afaict.

I pushed a tentative fix for the bot failure,
alas I'm completely unable to reproduce the issue locally, even
though it does not appear to be architecture dependant afaict.

It seems like those are the Sony CI bots... They default to C++14 (and a slightly different ABI) so if they fail alone usually it means someone didn't add a -std flag to the test.

I pushed a tentative fix for the bot failure,
alas I'm completely unable to reproduce the issue locally, even
though it does not appear to be architecture dependant afaict.

It seems like those are the Sony CI bots... They default to C++14 (and a slightly different ABI) so if they fail alone usually it means someone didn't add a -std flag to the test.

Thanks! this is super useful information and I can now reproduce locally,
Investigating ( the test crashes, so it's not just a missing flag )

cor3ntin updated this revision to Diff 473198.Nov 4 2022, 5:15 AM

Make sure cleanup expressions are correctly
added to rewritten initializers (I'm assuming this
did not manifest in C++20 mode because of copy elision)

cor3ntin updated this revision to Diff 473202.Nov 4 2022, 5:31 AM

Cleaner fix:

Only use MaybeCreateExprWithCleanups for member inits.
For default arguments, this is already handled in
ConvertParamDefaultArgument.

cor3ntin updated this revision to Diff 473213.Nov 4 2022, 6:28 AM

clamg-format

This revision was landed with ongoing or failed builds.Nov 4 2022, 6:46 AM
Closed by commit rG7acfe3629479: Implement CWG2631 (authored by cor3ntin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Hi, we're hitting an assertion failure after this patch

llvm-project/clang/lib/Sema/SemaDecl.cpp:15589: Decl *clang::Sema::ActOnFinishFunctionBody(Decl *, Stmt *, bool): Assertion `MaybeODRUseExprs.empty() && "Leftover expressions for odr-use checking"' failed.

Here is a reproducer https://storage.googleapis.com/fuchsia-artifacts/builds/8798414214460746113/build-debug/clang-crashreports/symbol_server_unittest-48a521.cpp and https://storage.googleapis.com/fuchsia-artifacts/builds/8798414214460746113/build-debug/clang-crashreports/symbol_server_unittest-48a521.sh unfortunately creduce was taking too long before I gave up on it, so sorry it hasn't been reduced at all.

I've verified that with this reverted the problem goes away. Could we revert unless there is a quick fix forward?

cor3ntin added a comment.EditedNov 4 2022, 1:49 PM

Hi, we're hitting an assertion failure after this patch

llvm-project/clang/lib/Sema/SemaDecl.cpp:15589: Decl *clang::Sema::ActOnFinishFunctionBody(Decl *, Stmt *, bool): Assertion `MaybeODRUseExprs.empty() && "Leftover expressions for odr-use checking"' failed.

Here is a reproducer https://storage.googleapis.com/fuchsia-artifacts/builds/8798414214460746113/build-debug/clang-crashreports/symbol_server_unittest-48a521.cpp and https://storage.googleapis.com/fuchsia-artifacts/builds/8798414214460746113/build-debug/clang-crashreports/symbol_server_unittest-48a521.sh unfortunately creduce was taking too long before I gave up on it, so sorry it hasn't been reduced at all.

I've verified that with this reverted the problem goes away. Could we revert unless there is a quick fix forward?

Thanks for reporting that. Did you tried a more recent commit? We fixed that issue today. (Along with z couple additional issues)

Hi, we're hitting an assertion failure after this patch

llvm-project/clang/lib/Sema/SemaDecl.cpp:15589: Decl *clang::Sema::ActOnFinishFunctionBody(Decl *, Stmt *, bool): Assertion `MaybeODRUseExprs.empty() && "Leftover expressions for odr-use checking"' failed.

Here is a reproducer https://storage.googleapis.com/fuchsia-artifacts/builds/8798414214460746113/build-debug/clang-crashreports/symbol_server_unittest-48a521.cpp and https://storage.googleapis.com/fuchsia-artifacts/builds/8798414214460746113/build-debug/clang-crashreports/symbol_server_unittest-48a521.sh unfortunately creduce was taking too long before I gave up on it, so sorry it hasn't been reduced at all.

I've verified that with this reverted the problem goes away. Could we revert unless there is a quick fix forward?

Thanks for reporting that. Did you tried a more recent commit? We fixed that issue today. (Along with z couple additional issues)

Yes, this happens at HEAD (currently ba65584d)

Fascinating. Looking more at the error it seems slightly different than what we fixed this morning.
I'll revert soon when I'm at a computer. Feel free to do it earlier if it's urgent for you
Sorry for the inconvenience.

Reverting now, just re-running the tests to be sure

@abrachet Reverted, sorry again
@aaron.ballman @shafik feel free to investigate the issue, i won't have the opportunity over the next two weeks

cor3ntin reopened this revision.Nov 4 2022, 2:30 PM
This revision is now accepted and ready to land.Nov 4 2022, 2:30 PM
cor3ntin updated this revision to Diff 473341.Nov 4 2022, 2:30 PM

Reopening with subsequent changes merged.

dblaikie added subscribers: rsmith, dblaikie.EditedNov 4 2022, 4:14 PM

fwiw, @rsmith came up with a crasher reproducer from this patch here:

template<typename T> struct F {
    template<typename U> F(const U&) {}
};

struct A {
    static constexpr auto x = [] {};
    F<int> f = x;
};

void f(A a = A()) { }

int main() {
    f();
}

This was still failing at52ffc728181bc2d3c889f7f80c252c3433b9e7b6 immediately prior to the revert.

Looks like it's distinct from the other reported crash, this one crashes with:

clang-16: /usr/local/google/home/blaikie/dev/llvm/src/clang/lib/CodeGen/CGExpr.cpp:2811: clang::CodeGen::LValue clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(const clang::DeclRefExpr *): Assertion `(ND->isUsed(false) || !isa<VarDecl>(ND) || E->isNonOdrUse() || !E->getLocation().isValid()) && "Should not use decl without marking it used!"' failed.

fwiw, @rsmith came up with a crasher reproducer from this patch here:

template<typename T> struct F {
    template<typename U> F(const U&) {}
};

struct A {
    static constexpr auto x = [] {};
    F<int> f = x;
};

void f(A a = A()) { }

int main() {
    f();
}

This was still failing at52ffc728181bc2d3c889f7f80c252c3433b9e7b6 immediately prior to the revert.

This is very helpful, thanks to both Richard and you.
Both issues seem to be related somewhat, there are scenarios in which entities are not marked odr used properly.
The other bug is still reducing, hopefully I'll have a test case for it tomorrow. I don't expect to be able to fix the issue in the next couple of weeks though

reduced to

struct a {
} constexpr b;
class c {
public:
  c(a);
};
class d {
  d() {}
  c e = b;
}
cor3ntin updated this revision to Diff 477003.Nov 21 2022, 2:23 PM

Properly mark default members initializers as ODR-used.

default members initializers were marked as maybe odr-used
but never actually odr-used.

We fix that by checking that an initializer consitutes
a full expression at the point it is used - which will in turn
handle cleanups and odr-use marking properly.

(This needs to be done after MarkDeclarationsReferencedInExpr,
which took me a while to fully figure out)

The changes generally LGTM, but I'd appreciate a second set of eyes on the changes just to double-check we're implementing the resolution properly.

clang/include/clang/AST/ExprCXX.h
1273

Do we need to update computeDependence() to account for the rewritten Expr *?

clang/include/clang/Sema/Sema.h
1344

It took me a moment to realize this wasn't the end of the structure declaration. :-D

9678–9679

We repeat this pattern four times in this patch; perhaps we should make a helper function for this predicate? Not certain of the best name for it, but removing the duplication might not be a bad idea.

cor3ntin updated this revision to Diff 478216.Nov 28 2022, 6:27 AM

Remove extra ;

clang/include/clang/AST/ExprCXX.h
1273

Looking at the code again, computeDependence do use the rewritten expr if there is one already (contracticting what I previously stated offline)

clang/include/clang/Sema/Sema.h
9678–9679

isNotPotentiallyEvaluatedContext? Something like that

cor3ntin updated this revision to Diff 478328.Nov 28 2022, 12:24 PM

Update DR status

cor3ntin added inline comments.Nov 29 2022, 4:41 AM
clang/include/clang/Sema/Sema.h
9678–9679

Actually, Maybe we should leave it as is in this patch,
and clean it up as part of https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2564r0.html
IE, I'm not sure i can come up with a completely correct name now.

aaron.ballman accepted this revision.Nov 29 2022, 8:26 AM

LGTM!

clang/include/clang/Sema/Sema.h
9678–9679

That's fine by me; naming is hard.

This revision was landed with ongoing or failed builds.Nov 30 2022, 2:25 AM
Closed by commit rG26fa17ed2914: Implement CWG2631 (authored by cor3ntin). · Explain Why
This revision was automatically updated to reflect the committed changes.
cor3ntin marked 2 inline comments as done.

Thanks Aaron. I hope it sticks this time!

cor3ntin reopened this revision.Nov 30 2022, 7:08 AM

There is still an ODR issue which caused linkers errors
I think I managed to reduce the issue to

template <typename T>
struct function_ref {
    function_ref(auto) {}
};

struct S {
    function_ref<int> X = nullptr;
};

void Do(S = {}) {
}

int main() {
    Do();
}

We mark nullptr odr used but that does not mark function_ref<int>::function_ref() ODR used.
We should make sure that's properly marked once the constructor call is resolved.

This revision is now accepted and ready to land.Nov 30 2022, 7:08 AM
cor3ntin updated this revision to Diff 479395.Dec 1 2022, 1:03 PM
  • Member initializers wwere not marked odr used properly
  • Fixing that unearthed an other bug: consteval calls in a nested member initializers were not properly detected if the member was list initialized to its default value (= {}).

@aaron.ballman if you don't scream, I'll probably try again this week end. I did multiple stage 2 full builds to be sure this time. The fix is pretty small but the investigation took a while...

aaron.ballman accepted this revision.Dec 7 2022, 11:16 AM

The new changes LGTM as well and the bots seem to be happy with things.

This revision was automatically updated to reflect the committed changes.

the following now produces a link error:

$ cat /tmp/a.cc
#include <array>
#include <map>

struct S {
        std::map<int, int> a;
};

using T = std::array<S, 20>;

class C {
        T t{};
};

int main() {
        C c;
}
$ ./build/rel/bin/clang++ -o /dev/null /tmp/a.cc -stdlib=libc++ -fuse-ld=lld
ld.lld: error: undefined hidden symbol: std::__2::map<int, int, std::__2::less<int>, std::__2::allocator<std::__2::pair<int const, int>>>::map[abi:v160000]()
>>> referenced by a.cc
>>>               /tmp/a-042a0e.o:(C::C())
clang: error: linker command failed with exit code 1 (use -v to see invocation)

using a ToT libc++

is this expected?

cor3ntin reopened this revision.EditedDec 8 2022, 12:34 PM

the following now produces a link error:

Thanks for reporting that. It was definitively not expected, I reverted the patch and will have a fix shortly.

This revision is now accepted and ready to land.Dec 8 2022, 12:34 PM
cor3ntin updated this revision to Diff 481411.Dec 8 2022, 12:42 PM

Correctly visit array filer when marking
default member init as ODR-used

This revision was landed with ongoing or failed builds.Dec 9 2022, 1:26 AM
Closed by commit rGc9a6713b4788: Implement CWG2631 (authored by cor3ntin). · Explain Why
This revision was automatically updated to reflect the committed changes.
aeubanks reopened this revision.Dec 9 2022, 2:20 PM

I've bisected another crash to this change. Reverting, and currently creducing a repro.

This revision is now accepted and ready to land.Dec 9 2022, 2:20 PM

a really bad reduced repro which also has random syntax errors:

using string = struct basic_string {
  constexpr basic_string(char *) {}                 constexpr ~basic_string(
} struct AutocompleteParsingResult struct optional {
  template <typename U = AutocompleteParsingResult> constexpr optional(U &&) {}
  namespace enum { kName } struct AutocompleteParsingResult {
    string StringPiece;
    optional expected_result;
    int max_length = 0
  } kAutocompleteTestcases {
    "", {
      { "", kName }
    }
}

clang -cc1 -x c++ -triple x86_64-unknown-linux-gnu -std=c++20 -fsyntax-only /tmp/reduced.cpp.orig -ferror-limit 1
working on a reduced repro with -ferror-limit=1

Thanks!

Cleaned up version:

struct string {
  constexpr string(const char*) {};
  constexpr ~string();
};
struct AutocompleteParsingResult;
struct optional {
    template <typename U = AutocompleteParsingResult> 
    constexpr optional(U &&) {}
};
struct AutocompleteParsingResult {
    string StringPiece;
    optional expected_result;
    int max_length = 0;
} kAutocompleteTestcases {
    "", {
        { "", 0 }
    }
};

And yes, this is about as reduced as I can make it.
I'll investigate later....

cor3ntin updated this revision to Diff 481850.Dec 10 2022, 5:50 AM
  • Make sure cleanups expressions are created in the correct evaluation context. This simplifies and reduce the numbers of nested evaluation contexts we create, notably to make sure the creation of the initialier and cleanup expressions are done in the same context
  • Add tests for calls to source location builtin in dependant initializers

@aeubanks I think i got it. Were you having this issue with an open source project by any chance? Maybe i could try it locally before commiting

yes, it was chrome
I went ahead and tried the latest patch, it successfully compiles the file that crashed before. I built all of chrome, and now I'm getting one last linker error, I'll try to get some more info about that

yes, it was chrome
I went ahead and tried the latest patch, it successfully compiles the file that crashed before. I built all of chrome, and now I'm getting one last linker error, I'll try to get some more info about that

Thanks for confirming :)
If you have the message of that linker error, i might be able to infer a repro from that. Otherwise, I'm going to build chromium

yes, it was chrome
I went ahead and tried the latest patch, it successfully compiles the file that crashed before. I built all of chrome, and now I'm getting one last linker error, I'll try to get some more info about that

Thanks for confirming :)
If you have the message of that linker error, i might be able to infer a repro from that. Otherwise, I'm going to build chromium

I got
x86_64-linux-gnu/libnspr4.so: undefined reference to gettid

But otherwise it works fine.
linking with mold fixes the issue.
I can't find an instance of gettid() that would be called from a default parameter or mem initializer list so I would be surprise if it's related. Was that the issue you encountered?

no, I got

$ ninja -C out/MyClang/ content_unittests
ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForDefer, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForDefer>>::Receiver(content::mojom::TestInterfaceForDefer*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:90 (../../content/browser/mojo_binder_policy_applier_unittest.cc:90)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())

ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForGrant, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForGrant>>::Receiver(content::mojom::TestInterfaceForGrant*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:91 (../../content/browser/mojo_binder_policy_applier_unittest.cc:91)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())

ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForCancel, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForCancel>>::Receiver(content::mojom::TestInterfaceForCancel*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:92 (../../content/browser/mojo_binder_policy_applier_unittest.cc:92)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())

ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForUnexpected, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForUnexpected>>::Receiver(content::mojom::TestInterfaceForUnexpected*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:93 (../../content/browser/mojo_binder_policy_applier_unittest.cc:93)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

https://crsrc.org/c/content/browser/mojo_binder_policy_applier_unittest.cc;drc=4e1b7bc33d42b401d7d9ad1dcba72883add3e2af;l=90

no, I got

$ ninja -C out/MyClang/ content_unittests
ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForDefer, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForDefer>>::Receiver(content::mojom::TestInterfaceForDefer*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:90 (../../content/browser/mojo_binder_policy_applier_unittest.cc:90)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())

ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForGrant, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForGrant>>::Receiver(content::mojom::TestInterfaceForGrant*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:91 (../../content/browser/mojo_binder_policy_applier_unittest.cc:91)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())

ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForCancel, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForCancel>>::Receiver(content::mojom::TestInterfaceForCancel*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:92 (../../content/browser/mojo_binder_policy_applier_unittest.cc:92)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())

ld.lld: error: undefined symbol: mojo::Receiver<content::mojom::TestInterfaceForUnexpected, mojo::RawPtrImplRefTraits<content::mojom::TestInterfaceForUnexpected>>::Receiver(content::mojom::TestInterfaceForUnexpected*)
>>> referenced by mojo_binder_policy_applier_unittest.cc:93 (../../content/browser/mojo_binder_policy_applier_unittest.cc:93)
>>>               obj/content/test/content_unittests/mojo_binder_policy_applier_unittest.o:(content::TestReceiverCollector::TestReceiverCollector())
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

https://crsrc.org/c/content/browser/mojo_binder_policy_applier_unittest.cc;drc=4e1b7bc33d42b401d7d9ad1dcba72883add3e2af;l=90

Thanks, that was really helpful.
After most of the day spent on compiling clang and reducing, i got it down to

template <typename> 
struct MissingCtr {
  MissingCtr() {}
};
class k {
//public:
  MissingCtr<k> a{};
};
struct b {
  k c{};
};
int main() { b d; }

It would appear to be related to access. I had not yet encountered something access related with this patch!

cor3ntin updated this revision to Diff 482051.Dec 12 2022, 2:55 AM

When a CXXDefaultInitExpr calls a Constructor,
make sure that the fields default initialized by that
constructor are also marked odr used.

@aeubanks I just completed a compilation of all of chrome including tests. let me know if you want to run more tests on your side and thanks again for reporting these issues. I think we are getting there.

I'm fine with relanding and seeing if anything else pops up

dankm added a subscriber: dankm.Dec 12 2022, 3:13 PM
dankm added inline comments.
clang/include/clang/Sema/Sema.h
9697

llvm::None has been deprecated in favour of std::nullopt.

cor3ntin updated this revision to Diff 482377.Dec 13 2022, 12:09 AM

remove llvm::None

This revision was landed with ongoing or failed builds.Dec 13 2022, 12:57 AM
Closed by commit rGf1f1b60c7ba6: Implement CWG2631 (authored by cor3ntin). · Explain Why
This revision was automatically updated to reflect the committed changes.

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.

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.

Oh no! I'd love a reduced repro if you can. Or at worse, a stack trace, I'd be happy to investigate. Thanks!

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:

struct SourceLocation {
  static SourceLocation current(const char * = __builtin_FILE()) {
    struct Metadata {
      Metadata(SourceLocation = current());
      namespace struct {
        int x = z(x, Metadata())
      } y {
      }

stack trace:

clang: /home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:752: const ExtQualsTypeCommonBase *clang::QualType::getCommonPtr() const: Assertion `!isNull() && "Cannot retrieve a NULL type pointer"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: bin/clang -c /tmp/repro.cc -std=c++17 -o /tmp/repro.o
1.      <eof> parser at end of file
2.      /tmp/repro.cc:1:1: parsing struct/union/class body 'SourceLocation'
3.      /tmp/repro.cc:2:66: parsing function body 'SourceLocation::current'
4.      /tmp/repro.cc:2:66: in compound statement ('{}')
 #0 0x00005645aaceaaba llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/rupprecht/src/llvm-project/llvm/lib/Support/Unix/Signals.inc:567:11
 #1 0x00005645aaceac6b PrintStackTraceSignalHandler(void*) /home/rupprecht/src/llvm-project/llvm/lib/Support/Unix/Signals.inc:641:1
 #2 0x00005645aace92f6 llvm::sys::RunSignalHandlers() /home/rupprecht/src/llvm-project/llvm/lib/Support/Signals.cpp:104:5
 #3 0x00005645aacea3ae llvm::sys::CleanupOnSignal(unsigned long) /home/rupprecht/src/llvm-project/llvm/lib/Support/Unix/Signals.inc:367:1
 #4 0x00005645aac01187 (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) /home/rupprecht/src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:0:7
 #5 0x00005645aac015dc CrashRecoverySignalHandler(int) /home/rupprecht/src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:398:1
 #6 0x00007f34de23daa0 (/lib/x86_64-linux-gnu/libc.so.6+0x3daa0)
 #7 0x00007f34de28957c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #8 0x00007f34de23da02 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #9 0x00007f34de228469 abort ./stdlib/abort.c:81:7
#10 0x00007f34de228395 _nl_load_domain ./intl/loadmsgcat.c:1177:9
#11 0x00007f34de236ab2 (/lib/x86_64-linux-gnu/libc.so.6+0x36ab2)
#12 0x00005645ab1dae17 clang::QualType::getCommonPtr() const /home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:0:5
#13 0x00005645ab1dada5 clang::QualType::getTypePtr() const /home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:6622:26
#14 0x00005645ab1da995 clang::QualType::operator->() const /home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:794:5
#15 0x00005645b071fe92 clang::computeDependence(clang::CXXThisExpr*) /home/rupprecht/src/llvm-project/clang/lib/AST/ComputeDependence.cpp:312:43
#16 0x00005645aed30c16 clang::CXXThisExpr::CXXThisExpr(clang::SourceLocation, clang::QualType, bool) /home/rupprecht/src/llvm-project/clang/include/clang/AST/ExprCXX.h:1154:19
#17 0x00005645af7f7c53 clang::Sema::BuildCXXThisExpr(clang::SourceLocation, clang::QualType, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1400:30
#18 0x00005645af7d46a8 clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::RebuildCXXThisExpr(clang::SourceLocation, clang::QualType, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:3196:22
#19 0x00005645af7b09ae clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformCXXThisExpr(clang::CXXThisExpr*) /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:12139:23
#20 0x00005645af7a9ec5 clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformExpr(clang::Expr*) /home/rupprecht/src/llvm-build/dev/tools/clang/include/clang/AST/StmtNodes.inc:907:1
#21 0x00005645af7b5097 clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformMemberExpr(clang::MemberExpr*) /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:11224:34
#22 0x00005645af7aa857 clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformExpr(clang::Expr*) /home/rupprecht/src/llvm-build/dev/tools/clang/include/clang/AST/StmtNodes.inc:1249:1
#23 0x00005645af7b945a clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformRecoveryExpr(clang::RecoveryExpr*) /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:10929:36
#24 0x00005645af7aade4 clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformExpr(clang::Expr*) /home/rupprecht/src/llvm-build/dev/tools/clang/include/clang/AST/StmtNodes.inc:1431:1
#25 0x00005645af71b9fe clang::TreeTransform<EnsureImmediateInvocationInDefaultArgs>::TransformInitializer(clang::Expr*, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:4006:25
#26 0x00005645af533e03 clang::Sema::BuildCXXDefaultInitExpr(clang::SourceLocation, clang::FieldDecl*) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaExpr.cpp:6090:19
#27 0x00005645af995bcf (anonymous namespace)::InitListChecker::FillInEmptyInitForField(unsigned int, clang::FieldDecl*, clang::InitializedEntity const&, clang::InitListExpr*, bool&, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaInit.cpp:688:32
#28 0x00005645af98aac1 (anonymous namespace)::InitListChecker::FillInEmptyInitializations(clang::InitializedEntity const&, clang::InitListExpr*, bool&, clang::InitListExpr*, unsigned int, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaInit.cpp:832:9
#29 0x00005645af972d8f (anonymous namespace)::InitListChecker::InitListChecker(clang::Sema&, clang::InitializedEntity const&, clang::InitListExpr*, clang::QualType&, bool, bool, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaInit.cpp:979:28
#30 0x00005645af97ebe3 clang::InitializationSequence::Perform(clang::Sema&, clang::InitializedEntity const&, clang::InitializationKind const&, llvm::MutableArrayRef<clang::Expr*>, clang::QualType*) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaInit.cpp:8555:27
#31 0x00005645af0b1a67 clang::Sema::AddInitializerToDecl(clang::Decl*, clang::Expr*, bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/SemaDecl.cpp:13027:33
#32 0x00005645aec9bb4a clang::Parser::ParseDeclarationAfterDeclaratorAndAttributes(clang::Declarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::ForRangeInit*) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDecl.cpp:0:15
#33 0x00005645aec99a98 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, clang::Parser::ForRangeInit*) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDecl.cpp:2196:9
#34 0x00005645aec98ae1 clang::Parser::ParseSimpleDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, bool, clang::Parser::ForRangeInit*, clang::SourceLocation*) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDecl.cpp:1893:10
#35 0x00005645aec98719 clang::Parser::ParseDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, clang::SourceLocation*) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDecl.cpp:1821:12
#36 0x00005645aec356d9 clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&, clang::ParsedAttributes&) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseStmt.cpp:247:16
#37 0x00005645aec3503b clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseStmt.cpp:115:20
#38 0x00005645aec3dca2 clang::Parser::ParseCompoundStatementBody(bool) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseStmt.cpp:1185:11
#39 0x00005645aec3f3a4 clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseStmt.cpp:2449:21
#40 0x00005645aecbb1b9 clang::Parser::ParseLexedMethodDef(clang::Parser::LexedMethod&) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseCXXInlineMethods.cpp:0:3
#41 0x00005645aecbac69 clang::Parser::LexedMethod::ParseLexedMethodDefs() /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseCXXInlineMethods.cpp:275:1
#42 0x00005645aecb9c5a clang::Parser::ParseLexedMethodDefs(clang::Parser::ParsingClass&) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseCXXInlineMethods.cpp:527:33
#43 0x00005645aec6b83f clang::Parser::ParseCXXMemberSpecification(clang::SourceLocation, clang::SourceLocation, clang::ParsedAttributes&, unsigned int, clang::Decl*) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:3629:5
#44 0x00005645aec69a34 clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:2103:7
#45 0x00005645aeca0586 clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseDecl.cpp:4225:23
#46 0x00005645aec05f12 clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*) /home/rupprecht/src/llvm-project/clang/include/clang/Parse/Parser.h:2417:5
#47 0x00005645aebff146 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /home/rupprecht/src/llvm-project/clang/lib/Parse/Parser.cpp:1120:7
#48 0x00005645aebfec6f clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /home/rupprecht/src/llvm-project/clang/lib/Parse/Parser.cpp:1222:12
#49 0x00005645aebfe53f clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) /home/rupprecht/src/llvm-project/clang/lib/Parse/Parser.cpp:1037:14
#50 0x00005645aebfc406 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /home/rupprecht/src/llvm-project/clang/lib/Parse/Parser.cpp:743:12
#51 0x00005645aebfbac0 clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /home/rupprecht/src/llvm-project/clang/lib/Parse/Parser.cpp:591:8
#52 0x00005645aebf6f48 clang::ParseAST(clang::Sema&, bool, bool) /home/rupprecht/src/llvm-project/clang/lib/Parse/ParseAST.cpp:161:15
#53 0x00005645ac094226 clang::ASTFrontendAction::ExecuteAction() /home/rupprecht/src/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1164:1
#54 0x00005645ac270234 clang::CodeGenAction::ExecuteAction() /home/rupprecht/src/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:1170:5
#55 0x00005645ac093c4c clang::FrontendAction::Execute() /home/rupprecht/src/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1059:7
#56 0x00005645abfc5b5c clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /home/rupprecht/src/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1046:23
#57 0x00005645ac25ad37 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /home/rupprecht/src/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:264:8
#58 0x00005645a741eb00 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /home/rupprecht/src/llvm-project/clang/tools/driver/cc1_main.cpp:251:13
#59 0x00005645a740ca55 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /home/rupprecht/src/llvm-project/clang/tools/driver/driver.cpp:353:5
#60 0x00005645abe88bd5 clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const::$_1::operator()() const /home/rupprecht/src/llvm-project/clang/lib/Driver/Job.cpp:428:34
#61 0x00005645abe88ba5 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const::$_1>(long) /home/rupprecht/src/llvm-project/llvm/include/llvm/ADT/STLFunctionalExtras.h:45:5
#62 0x00005645a992ab49 llvm::function_ref<void ()>::operator()() const /home/rupprecht/src/llvm-project/llvm/include/llvm/ADT/STLFunctionalExtras.h:68:5
#63 0x00005645aac00f9a llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) /home/rupprecht/src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:434:3
#64 0x00005645abe8838b clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const /home/rupprecht/src/llvm-project/clang/lib/Driver/Job.cpp:428:7
#65 0x00005645abe2890f clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const /home/rupprecht/src/llvm-project/clang/lib/Driver/Compilation.cpp:199:15
#66 0x00005645abe28b17 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&, bool) const /home/rupprecht/src/llvm-project/clang/lib/Driver/Compilation.cpp:253:13
#67 0x00005645abe41d58 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&) /home/rupprecht/src/llvm-project/clang/lib/Driver/Driver.cpp:1817:7
#68 0x00005645a740c4df clang_main(int, char**) /home/rupprecht/src/llvm-project/clang/tools/driver/driver.cpp:555:9

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.

cor3ntin reopened this revision.Dec 14 2022, 3:17 PM
This revision is now accepted and ready to land.Dec 14 2022, 3:17 PM

Here's a well-formed reproducer:

struct MyStringView {
  MyStringView(const char *);
};

struct SourceLocation {
  static SourceLocation current(const char * = __builtin_FILE(),
                                unsigned int = __builtin_LINE());
};

template <typename T>
int Wrap(T);

template <typename>
struct Fixed {
  using name = MyStringView;
};

struct Metadata {
  Metadata(SourceLocation = SourceLocation::current());
};

struct Name {
  Name(int *);
};

template <typename... Fields>
struct C {
  static C New(Name, typename Fixed<Fields>::name..., Metadata);
};
struct X {
  int &x();
};
struct Foo {
  X x;
  int i = Wrap(C<int, int>::New(&x.x(), "", "", Metadata()));
};
void func() { new Foo{}; }

Here's a well-formed reproducer:

Thanks a lot.
We were transforming the initializer in a context where the this pointer was not set up properly, so if it was referring to this, explicitly or implicitly, it would crash.

cor3ntin updated this revision to Diff 483133.Dec 15 2022, 4:55 AM

Properly transform the this pointer in member initializers
that need to be rewritten.

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

However, now I get this inexplicable error -- I'm not entirely sure it's related, maybe I'm holding it wrong:

In module '<foo>':
foo.h$line:$num: error: 'foo::FooClass' has different definitions in different modules; first difference is definition in module 'something.h' found data member 'kFooDelimiter' with an initializer
  static constexpr char kFooDelimiter = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' with a different initializer
  static constexpr char kFooDelimiter = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

The definition seems straightforward:

class FooClass final {
  ...
  static constexpr char kFooDelimiter = '+';
  ...
};
cor3ntin added a comment.EditedDec 16 2022, 1:41 AM

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

However, now I get this inexplicable error -- I'm not entirely sure it's related, maybe I'm holding it wrong:

In module '<foo>':
foo.h$line:$num: error: 'foo::FooClass' has different definitions in different modules; first difference is definition in module 'something.h' found data member 'kFooDelimiter' with an initializer
  static constexpr char kFooDelimiter = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' with a different initializer
  static constexpr char kFooDelimiter = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

The definition seems straightforward:

class FooClass final {
  ...
  static constexpr char kFooDelimiter = '+';
  ...
};

This is *very* surprising to me.
I could explain it if the member was not static though, as it would be the kind of things the patch affects. But static data members are handled very differently.

Was that liking chrome? It didn't come up in my tests

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

$ CLANG=~/dev/clang ./repro.sh
++ dirname /tmp/repro/repro.sh
+ DIR=/tmp/repro
+ : /tmp/D136554
+ rm -rf /tmp/D136554
+ mkdir -p /tmp/D136554
+ cd /tmp/D136554
+ cp /tmp/repro/lib.h /tmp/repro/outer.h /tmp/repro/x.h /tmp/repro/y.h /tmp/repro/z.h /tmp/repro/lib.cppmap /tmp/repro/outer.cppmap /tmp/repro/x.cppmap /tmp/repro/y.cppmap /tmp/repro/z.cppmap .
+ COMMON_ARGS=('-Wno-mismatched-tags' '-Xclang=-fmodules-embed-all-files' '-Xclang=-fmodules-local-submodule-visibility' '-fmodules' '-fno-implicit-modules' '-fno-implicit-module-maps' '-std=gnu++17' '-Xclang=-fmodule-map-file-home-is-cwd')
+ export COMMON_ARGS
+ /home/rupprecht/dev/clang -Wno-mismatched-tags -Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility -fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 -Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=x -fmodule-map-file=x.cppmap -fmodule-map-file=lib.cppmap -xc++ -Xclang=-emit-module -c x.cppmap -o x.pcm
+ /home/rupprecht/dev/clang -Wno-mismatched-tags -Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility -fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 -Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=y -fmodule-map-file=y.cppmap -fmodule-map-file=lib.cppmap -fmodule-map-file=z.cppmap -xc++ -Xclang=-emit-module -c y.cppmap -o y.pcm
+ /home/rupprecht/dev/clang -Wno-mismatched-tags -Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility -fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 -Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=outer -fmodule-map-file=outer.cppmap -Xclang=-fmodule-file=x.pcm -Xclang=-fmodule-file=y.pcm -fmodule-map-file=lib.cppmap -fmodule-map-file=x.cppmap -fmodule-map-file=y.cppmap -xc++-header -fsyntax-only -c outer.h -o outer.h.processed
In module 'x':
./lib.h:5:25: error: 'Foo' has different definitions in different modules; first difference is definition in module 'x.x.h' found data member 'kConstant' with an initializer
  static constexpr char kConstant = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
./lib.h:5:25: note: but in 'y.y.h' found data member 'kConstant' with a different initializer
  static constexpr char kConstant = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
1 error generated.

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

However, now I get this inexplicable error -- I'm not entirely sure it's related, maybe I'm holding it wrong:

In module '<foo>':
foo.h$line:$num: error: 'foo::FooClass' has different definitions in different modules; first difference is definition in module 'something.h' found data member 'kFooDelimiter' with an initializer
  static constexpr char kFooDelimiter = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' with a different initializer
  static constexpr char kFooDelimiter = '+';
  ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

The definition seems straightforward:

class FooClass final {
  ...
  static constexpr char kFooDelimiter = '+';
  ...
};

This is *very* surprising to me.
I could explain it if the member was not static though, as it would be the kind of things the patch affects. But static data members are handled very differently.

Was that liking chrome? It didn't come up in my tests

No, it's some other internal target. There isn't any way for you to be able to test against these targets, so unfortunately the best I can offer is I'll patch in any updated versions of this patch, see what breaks, and try to reduce it when reporting.

There's barely any code in the repro I attached, it's just a bunch of header/module layering. My guess is that clang is usually able to see that the two definitions in different modules is the same and therefore dedupes them, but this change adds some unique bit of information that makes clang think it's different. Note that the headers need to be in a particular order to break.

cor3ntin updated this revision to Diff 484802.Dec 22 2022, 5:11 AM

Thanks @rupprecht

As all good bugs, this was very confusing up
until the moment it became obvious.

I did not make the initializer a full expression during parsing,
(but on use), as I thought that was unecessary.
But then if that initializer had any cleanup, the next expression
being parsed would get cleanups instead.

So here kFooDelimiter initializer would be wrapped in ExprWithCleanup
(it clearly should not), if an only if Xyz was parsed before hand.

I added an ast dump test for that case.

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

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.

Fully reduced as:

template <typename a, typename b> int c(a, b);
struct d {
  static d e(const char * = __builtin_FILE());
};
struct f {
  f(d = d::e());
};
struct h {
  int &g;
  int blah = c(g, f());
};
struct k {
  k();
  int i;
  h j{i};
};
k::k() {}

With the error: repro.cc:10:16: error: 'h::g' is not a member of class 'k'

cor3ntin updated this revision to Diff 485067.Dec 23 2022, 1:14 AM

I hope we will get there...

It further reduces to

consteval void immediate(){};

struct h {
  int g = 0;
  int blah = (immediate(), g);
};
struct k {
 h j{};
}_;

The issue was that nested expressions are all transformed in the context of
the outermost (k) class and transforming the this pointer pick up
the current this pointer.
Because when we do the replacenent we no longer have templates,
we can just skip the transformation of the this pointer.

This is a cleaner way to handke the this pointer than
having a CXXThisScopeRAII on the outer scope,
so I removed that.

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.

... and yep, I was holding it wrong. There are still build failures, this time in linking, where LLD reports some symbols are undefined. I'm not certain yet if this really a problem with this patch (e.g. sometimes we see files break like this when users put their template code in the .cpp files and not the .h files), but the source code doesn't look immediately wrong. Once again, I'll try to reduce something.

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>

struct Options {
  std::function<bool(bool x)> identity = [](bool x) -> bool { return x; };
};

struct Wrapper {
  explicit Wrapper(const Options& options = Options()) {}
};

void Func() { Options options; }

The object file reports many fewer symbols as a result of this patch, but one new problematic one. Before:

0000000000000000 T Func()
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::target_type() const
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::target(std::type_info const&) const
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::__clone(std::__1::__function::__base<bool (bool)>*) const
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::__clone() const
0000000000000000 W std::__1::__function::__base<bool (bool)>::~__base[abi:v160000]()
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::destroy_deallocate()
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::destroy()
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::~__func()
0000000000000000 W std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>::operator()(bool&&)
0000000000000000 V typeinfo for Options::identity::'lambda'(bool)
0000000000000000 V typeinfo for std::__1::__function::__base<bool (bool)>
0000000000000000 V typeinfo for std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>
0000000000000000 V typeinfo name for Options::identity::'lambda'(bool)
0000000000000000 V typeinfo name for std::__1::__function::__base<bool (bool)>
0000000000000000 V typeinfo name for std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>
                 U vtable for __cxxabiv1::__class_type_info
                 U vtable for __cxxabiv1::__si_class_type_info
0000000000000000 V vtable for std::__1::__function::__func<Options::identity::'lambda'(bool), std::__1::allocator<Options::identity::'lambda'(bool)>, bool (bool)>
                 U operator delete(void*)
                 U operator new(unsigned long)

And after:

0000000000000000 T Func()
                 U std::__1::function<bool (bool)>::function<Options::identity::'lambda'(bool), void>(Options::identity::'lambda'(bool))

The undefined symbols before are all provided by libc++, so those are fine. After, the new undefined symbol for the lambda cannot be resolved. Depending on how the linker is invoked, this may or may not be fine -- IIUC the linker can sometimes recognize that it doesn't actually need the undefined symbol, so it doesn't matter if it can't find it.

Here is the build script I'm using, although you will likely need to tweak it for your own environment:

${CLANG} \
  -std=gnu++17 -stdlib=libc++ \
  -c main.cc \
  -o /tmp/main.o

echo "main.o symbols:"
llvm-nm -C /tmp/main.o

echo Building b.cc

${CLANG} \
  -std=gnu++17 -stdlib=libc++ \
  -O1 -fno-exceptions \
  -c b.cc \
  -o /tmp/b.o

echo "b.o symbols:"
llvm-nm -C /tmp/b.o

echo Linking, and expecting success

${CLANG} \
  -fuse-ld=lld \
  -o /tmp/main \
  /tmp/main.o \
  -Wl,--start-lib /tmp/b.o -Wl,--end-lib \
  /usr/lib/x86_64-linux-gnu/libc++.so

echo Linking, and expecting failure with D136554

${CLANG} \
  -fuse-ld=lld \
  -o /tmp/main \
  /tmp/main.o \
  /tmp/b.o \
  /usr/lib/x86_64-linux-gnu/libc++.so \
  -Wl,--export-dynamic

(Here b.cc is the first snippet above, and main.cc is a trivial file with just int main(int argc, char* argv[]) { return 0; })

MaskRay added a subscriber: MaskRay.EditedDec 29 2022, 12:32 AM

[...]
The undefined symbols before are all provided by libc++, so those are fine. After, the new undefined symbol for the lambda cannot be resolved. Depending on how the linker is invoked, this may or may not be fine -- IIUC the linker can sometimes recognize that it doesn't actually need the undefined symbol, so it doesn't matter if it can't find it.

Here is the build script I'm using, although you will likely need to tweak it for your own environment:

The linker behavior is expected. The "undefined symbol" error does show a defect of this patch.

For /tmp/main.o -Wl,--start-lib /tmp/b.o -Wl,--end-lib, b.o is a lazy object file (with archive semantics) which is not extracted.
The result is as if the linker discards the input file, so its undefined references do not cause a linker error ([[ https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs | -z defs ]]: unresolved undefined non-weak symbol from a relocatable object file; an unextracted lazy file is not considered a relocatable object file ).

For /tmp/main.o /tmp/b.o, b.o is parsed as a relocatable object file. Its undefined reference causes by this patch (experiment locally with git revert -n 339a7687e1c036a5f91c9d5391523b93e2e76cd3) leads to a -z defs linker error.

cor3ntin updated this revision to Diff 485605.Dec 29 2022, 4:13 AM

Another fun one.
I really apreciate your help on this.

The constructor of Wrapper causes
the constructor of Option to be defined, but not yet used
(as default parameters are not odr used) -
so none of the default member initializers where marked ODR used.

When Option::Option() is called by Func, it is marked
as referenced and ODR used, but its default member initializers
are not.

When marking a constructor as referenced, we therefore must make sure
to reference its default member initializers.

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:

File blah.h:

#include <functional>
#include <memory>

template <typename T>
struct Base {
  virtual ~Base() = default;
};

class Impl : public Base<int> {};

struct ImplHolder {
  std::unique_ptr<Base<int>> impl = std::make_unique<Impl>();
};

struct GetImplCreators {
  std::function<ImplHolder()> impl_creator = []() { return ImplHolder{}; };
};

void UseImpl(GetImplCreators impl_creators = GetImplCreators{});

And blah.cc:

#include "blah.h"

void UseImpl(GetImplCreators impl_creators) {}

And lastly, blah_main.cc:

#include "blah.h"

void Func1() { UseImpl(); }
// Note: it also fails when we explicitly specify the default arg, in case that is interesting.
// void Func2() { UseImpl(GetImplCreators()); }

int main(int argc, char* argv[]) { return 0; }

And here's the LLD output:

ld: error: undefined hidden symbol: std::__u::__unique_if<Impl>::__unique_single std::__u::make_unique<Impl>()
>>> referenced by blah_main.cc
>>>               blah_main.o:(ImplHolder std::__u::__function::__policy_invoker<ImplHolder ()>::__call_impl<std::__u::__function::__default_alloc_func<GetImplCreators::impl_creator::'lambda'(), ImplHolder ()>>(std::__u::__function::__policy_storage const*))

ld: error: undefined hidden symbol: std::__u::unique_ptr<Impl, std::__u::default_delete<Impl>>::~unique_ptr()
>>> referenced by blah_main.cc
>>>               blah_main.o:(ImplHolder std::__u::__function::__policy_invoker<ImplHolder ()>::__call_impl<std::__u::__function::__default_alloc_func<GetImplCreators::impl_creator::'lambda'(), ImplHolder ()>>(std::__u::__function::__policy_storage const*))

I'm out of time today so I don't have a repro script to offer, but I can dig into that when I get back next year if the source-only example is insufficient.

cor3ntin updated this revision to Diff 485703.Dec 30 2022, 8:19 AM

Finding the issue took about all day.
I reduced your example to

template <typename a>
int templated_func() {
    return 0;
}
struct test_body {
  int mem = templated_func<int>();
};

struct S {
  int a = [] { test_body{}; return 0;}(); // #1
} s;

When parsing the lambda #1 , we were not considering test_body{} to be
odr used, on account of the lambda being in the default initializer of a.

However, as it turns out, expressions in the body of a lambda are always
considered ODR used even if the lambda appear in a default parameter,
which I think makes sense.
(as the standard specifies that the body of a lambda is not a sub expression)

The actual code fix is to simplify the check we do to see if we are
in a nested default initializer.

MaskRay added a comment.EditedDec 30 2022, 10:29 AM

@rupprecht You may consider contributing some interesting tests (which work before this patch) in a separate patch:)

@rupprecht You may consider contributing some interesting tests (which work before this patch) in a separate patch:)

Well, all the failures reported have been reduced and added as test cases.
The coverage for member initializers was a bit spotty, that's for sure!

rupprecht accepted this revision.Jan 3 2023, 5:38 PM

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;
}

// b.h
#include "a.h"

class X {
  void Foo(..., const std::pair<double, double>& x = GetFakePair()) const;
};

// main.cc, compiled with -Wunused-function
#include "b.h"
int main() { return 0; }

Yields this error:

In file included from main.cc:9:
In file included from ./b.h:16:
./a.h:40:41: error: 'static' function 'GetFakePair' declared in header file should be declared 'static inline' [-Werror,-Wunneeded-internal-declaration]
static const std::pair<double, double>& GetFakePair() {

Unless I'm missing something, it seems that, in regards to this warning, this change is just enabling an existing warning to have more coverage, and therefore this change looks ready to ship. Thanks for letting me provide all my repros before landing this one again :)

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 missed another place in the unreduced repro that was referencing this method, and that was the trick. I reduced the newly-enabled warning case to this:

// a.h
static const std::pair<double, double>& GetFakePairRef() {
  static constexpr std::pair<double, double> fake = {1.0, 2.0};
  return fake;
}

struct Metadata {
  const std::pair<double, double>& data = GetFakePairRef();
};

// main.cc, compiled with -Wunused-function
#include "a.h"
int main() { return 0; }

Again, still LGTM.

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 missed another place in the unreduced repro that was referencing this method, and that was the trick. I reduced the newly-enabled warning case to this:

// a.h
static const std::pair<double, double>& GetFakePairRef() {
  static constexpr std::pair<double, double> fake = {1.0, 2.0};
  return fake;
}

struct Metadata {
  const std::pair<double, double>& data = GetFakePairRef();
};

// main.cc, compiled with -Wunused-function
#include "a.h"
int main() { return 0; }

Again, still LGTM.

Yes, this is a nice improvement.
It make sense. We now only mark default member initializers odr used when they are actually odr used.
default constructing an instance of MetaData silences the warning, which is expected!

I'll probably land today.
Thanks a lot for helping me find and reduce all of these corner cases, it was very useful and appreciated :)

I found a new issue doing a bootstrap build, I'm currently reducing

cor3ntin added a comment.EditedJan 4 2023, 5:59 PM

reduced to this lovely thing

template <typename e, bool = __is_constructible(e)> struct f {};
template <class k, f<k>> int l;
int m;
struct p {
  int o = m;
  p() {}
};

int i(l<p, false>);

SemaExpr.cpp:19857: void DoMarkVarDeclReferenced(clang::Sema &, clang::SourceLocation, clang::VarDecl *, clang::Expr *, llvm::DenseMap<const VarDecl *, int> &): Assertion `(!E || isa<FunctionParmPackExpr>(E)) && "missing non-odr-use marking for unevaluated decl ref"' failed.

@rupprecht You may consider contributing some interesting tests (which work before this patch) in a separate patch:)

Well, all the failures reported have been reduced and added as test cases.
The coverage for member initializers was a bit spotty, that's for sure!

Thanks for checking! I was asking whether some tests worked before the patch and served as good regression tests. Then they can be pushed separately.

cor3ntin updated this revision to Diff 486508.Jan 5 2023, 2:49 AM

I've convinced myself that the assertition case should be removed.

Before this patch, CXXDefaultInitExpr where never visited
by UsedDeclVisitor.

After this patch, when doing that visitation,
a variable can appear in an unevaluated context without having been mark
non odr used (and doing so would require to unconditionally transform
default member initializers, which we want to avoid).

cor3ntin updated this revision to Diff 486510.Jan 5 2023, 2:50 AM

formatting

Unless something else pops up, I'll try to land that this week end.

This revision was landed with ongoing or failed builds.Jan 8 2023, 1:35 AM
Closed by commit rGca6196138012: Implement CWG2631 (authored by cor3ntin). · Explain Why
This revision was automatically updated to reflect the committed changes.
cebowleratibm added a subscriber: cebowleratibm.EditedJan 16 2023, 9:00 AM

I've reduced a regression on:

commit ca619613801233ef2def8c3cc7d311d5ed0033cb (HEAD, refs/bisect/bad)
Author: Corentin Jabot <corentinjabot@gmail.com>
Date: Sun Oct 23 17:32:58 2022 +0200

template <class T> int f(T) { return 42; }

struct A {
   int x = f(A());
   A() { }
};

void foo() { A(); }
clang++ t2.C -c
t2.C:4:12: warning: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely [-Wstack-exhausted]
   int x = f(A());
           ^
Segmentation fault (core dumped)

@cor3ntin would you like me to open a new issue? Admittedly the testcase is contrived but clang shouldn't seg fault on it.

Yes please! However the warning looks correct to me in that case. A
constructs x which constructs A etc.

Yes please! However the warning looks correct to me in that case. A
constructs x which constructs A etc.

https://github.com/llvm/llvm-project/issues/60082

Please set the project/labels as appropriate.

one more regression bisected to this: https://bugs.chromium.org/p/chromium/issues/detail?id=1408177
incomplete type 'blink::ResourceClient' used in type trait expression
I'll try to come up with a smaller repro

one more regression bisected to this: https://bugs.chromium.org/p/chromium/issues/detail?id=1408177
incomplete type 'blink::ResourceClient' used in type trait expression
I'll try to come up with a smaller repro

sorry, false alarm due to faulty bisection