This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Implement P2169 A nice placeholder with no name
ClosedPublic

Authored by cor3ntin on Jun 22 2023, 5:22 AM.

Details

Summary

This is a C++ feature that allows the use of _ to
declare multiple variable of that name in the same scope;
these variables can then not be referred to.

In addition, while P2169 does not extend to parameter
declarations, we stop warning on unused parameters of that name,
for consistency.

The feature is backported to all C++ language modes.

Diff Detail

Event Timeline

cor3ntin created this revision.Jun 22 2023, 5:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 5:22 AM
cor3ntin requested review of this revision.Jun 22 2023, 5:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 5:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cor3ntin edited the summary of this revision. (Show Details)Jun 22 2023, 5:23 AM
cor3ntin added a reviewer: Restricted Project.
tbaeder added inline comments.
clang/test/Lexer/unicode.c
30

Are these changes only for the case where we compile as c++? I know lots of C (and c++?) projects use _ as GNU gettext identifier to mark a translatable string.

cor3ntin added inline comments.Jun 23 2023, 2:06 AM
clang/test/Lexer/unicode.c
30

Are these changes only for the case where we compile as c++

Yes

I know lots of C (and c++?) projects use _ as GNU gettext identifier to mark a translatable string.

That's doesn't interfere with this feature. if _ is defined as a macro, it's simply not possible to declare a variable of that name, let alone more than one.

erichkeane added inline comments.
clang/lib/Sema/SemaDecl.cpp
2031

Why would we get here? Doesn't line 1995 pick this up? Or am I missing where D is modified?

ALSO, I'd suggest making this VD->isPlaceholderVar, as to not annoy the SA tools.

clang/lib/Sema/SemaDeclCXX.cpp
725

Not a fan of this function name... Perhaps Diag instead of Note?

shafik added a subscriber: shafik.Jun 23 2023, 9:19 PM
shafik added inline comments.
clang/lib/AST/Decl.cpp
1869

It is not obvious to me if this is a drive by fix or this has a larger effect in the context of this change. Is there a test that demonstrates the effect of this change?

shafik added inline comments.Jun 23 2023, 9:29 PM
clang/lib/Sema/SemaDecl.cpp
18285

Why can't we fold this into FieldDecl::Create? This comment applies in some other places as well.

cor3ntin updated this revision to Diff 534185.Jun 24 2023, 3:34 AM
  • Address Shafik's an Erich's comments
  • Add missing serialization code
cor3ntin marked 3 inline comments as done.Jun 24 2023, 3:39 AM
cor3ntin added inline comments.
clang/lib/AST/Decl.cpp
1869

That's a left over from when the patched allowed placeholder in parameter names (EWG voted against that). thanks!

clang/lib/Sema/SemaDecl.cpp
2031

Yep, this was dead code, thanks!

18285

By adding a parameter to FieldDecl::Create? We could, I'm not sure it would necessarily be cleaner. Let me know what you prefer.

cor3ntin updated this revision to Diff 534329.Jun 25 2023, 4:30 AM
cor3ntin marked 2 inline comments as done.

Rebase

ChuanqiXu added inline comments.Jun 25 2023, 7:32 PM
clang/lib/Serialization/ASTWriterDecl.cpp
2043 ↗(On Diff #534329)

In case IsPlaceholder must be 0 when use the abbreviation for FieldDecl, it is better to use Abv->Add(BitCodeAbbrevOp(0)); here.

2077 ↗(On Diff #534329)

ditto

2233 ↗(On Diff #534329)

ditto

2311 ↗(On Diff #534329)

ditto.

cor3ntin updated this revision to Diff 534455.Jun 26 2023, 1:10 AM

Address ChuanqiXu's feedback

cor3ntin planned changes to this revision.Jun 26 2023, 7:19 AM
cor3ntin updated this revision to Diff 534592.Jun 26 2023, 8:59 AM
  • Fix missed Decl abbreviation change
  • Fix feature macro test
shafik added inline comments.Jun 26 2023, 11:03 AM
clang/lib/Sema/SemaDecl.cpp
18285

It seems like having the code consolidated make for better maintainability and figuring this out for future folks but I am willing to be convinced otherwise.

aaron.ballman added inline comments.
clang/docs/ReleaseNotes.rst
135–136

Should we consider adding this as an extension to C? It would be conforming there as well because it wouldn't change the behavior of any correct C code, right?

clang/include/clang/AST/DeclBase.h
340–342 ↗(On Diff #534592)

This seems a bit heavy to put on every Decl, it only matters for declarations with names, right? Should this be on NamedDecl instead? And should it be using terminology like "name independent"?

Actually, do we need this bit at all? Would it work for NamedDecl to have a function that returns whether the declaration name is _ or not? Oh, I suppose we can't get away with that because void _(int x); does not introduce a placeholder identifier but a regular one, right?

clang/include/clang/Basic/DiagnosticSemaKinds.td
6593

I don't think this helps the user understand what's wrong with their code, especially given the somewhat odd language rules around the feature. How about: ambiguous reference to multiply-defined placeholder '_' or something along those lines? Then the note can show the previous declarations of the placeholders that are in scope? e.g.,

void g() {
int _; // note: placeholder declared here
_ = 0;
int _; // note: placeholder declared here
_ = 0; // error: `ambiguous reference to multiply-defined placeholder '_'`
}

CC @cjdb

clang/include/clang/Sema/Lookup.h
121
clang/lib/Sema/SemaDecl.cpp
2179–2180

What's the rationale here? I know the standard has a recommended practice, but I don't see why that should apply to the case where the user assigns a value to the placeholder but never references it otherwise.

7528

Please spell out the type here.

7529
7531–7533
8376
18285

We need to support partial construction because of AST deserialization, but it seems to me that we don't need to add a new parameter to FieldDecl::Create() anyway; can't we have the FieldDecl constructor look at the given IdentifierInfo * in that case, and if it's nonnull, set the placeholder from that?

clang/lib/Sema/SemaDeclCXX.cpp
886

assert(VarName && "Cannot have an unnamed binding declaration"); ?

918
clang/lib/Sema/SemaLambda.cpp
1151–1153
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
32

I don't understand what this diagnostic is trying to tell me, can you explain a bit more?

80

Weird.. is this expected? You can have two variables named _ in block scope, but you can't have two parameters named _ despite them inhabiting the same block scope?

cjdb added inline comments.Jun 27 2023, 9:37 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
6593

Agreed. I'd suggest a rewording though: I took "multiply" to mean the maths term until completing the sentence, rather than its alternative meaning of "multiple instances" (which is more or less the same meaning, but "multiply" maps to the x * y operation for me).

Perhaps ambiguous reference to placeholder '_', which has multiple definitions? Not sold on that being the best wording, but it does avoid the hardcoded-word-at-8yo problem :)

cor3ntin marked 14 inline comments as done.Jun 27 2023, 11:41 AM
cor3ntin added inline comments.
clang/docs/ReleaseNotes.rst
135–136

Maybe we should consult WG14 first?
It's of very questionable usefulness without structured bindings and destructors.

clang/include/clang/AST/DeclBase.h
340–342 ↗(On Diff #534592)

I think you made me realize we can probably compute that completely without using bits. Oups.
It would certainly simplify things. I'll investigate!

clang/include/clang/Basic/DiagnosticSemaKinds.td
6593

@cjdb I like your suggestion. maybe "which is defined multiple times?" - pedantically a bunch of things have been defined once each, it's not a redefinition of the same entity in the c++ sense

clang/lib/Sema/SemaDecl.cpp
2179–2180

The intent is that this behave like [[maybe_unused]]

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
32

The no side effect one?
You are assigning something that has no side effect to something that you may never be able to name, so the whole thing is likely dead code.
I welcome improvements suggestions

80

Yes, CWG decided against having it in parameter declarations, because otherwise we'd need them in template parameters which we didn't want to do.
And it would be useless, you can simply not name them.

cor3ntin updated this revision to Diff 535089.Jun 27 2023, 11:51 AM
cor3ntin marked 2 inline comments as done.

Address some of Aarons comments before getting rid of the bit in Decl

aaron.ballman added inline comments.Jun 27 2023, 1:04 PM
clang/docs/ReleaseNotes.rst
135–136

We certainly could ask WG14 for their opinion, but the scenario I was thinking this would help with is for complex macros where variables are sometimes defined but the names are not important as they shouldn't escape. However, I think those scenarios generally introduce different scopes (often with GNU statement expressions) so there's not a naming conflict that we'd need to avoid. So I'm fine skipping C for now until we hit a concrete use case.

clang/include/clang/AST/DeclBase.h
340–342 ↗(On Diff #534592)

Thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td
6593

I like either of your suggestions better than my use of "multiply-defined." :-)

clang/lib/Sema/SemaDecl.cpp
2179–2180

Ah, thank you! I think we should make a slightly wider change here then. Let's move VD->hasAttr<UnusedAttr>() out of the first if statement and into this one, then add a comment about the relationship between the attribute and this feautre. WDYT?

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
32

Yeah, the no side effect one.

Corentin and I discussed this off-list and I understand the situation a bit better now. Basically, an init capture named _ either has a side effect on construction and/or destruction (like an RAII object) or it cannot be used within the lambda. However, it does still have effects (for example, it impacts whether the lambda has a function conversion operator).

That makes this kind of tricky to word. Given that it's a QoI warning, I'd recommend we leave this bit for a follow-up so that we can land the majority of the functionality and we can add the safety rails later.

80

I guess I see the same amount of utility in allowing an init capture named _ as I do a function parameter named _ as a local variable named _ (if it is an RAII type, it can do useful things in theory; otherwise, it's not a useful declaration).

cor3ntin updated this revision to Diff 535116.Jun 27 2023, 1:32 PM
  • Address more feedback
  • reword diag
  • remove diag for unused init capture
  • Remove the IsPlaceholder bit, as placeholder variables can be determined without data
cor3ntin marked 14 inline comments as done.Jun 27 2023, 1:34 PM
cor3ntin added inline comments.
clang/include/clang/AST/DeclBase.h
340–342 ↗(On Diff #534592)

Removed!

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
32

Removed

80

side effects are the main motivation for local variables beside structured bindings (and pattern matching later)

I've not spotted anything major, but I would like some additional test coverage. Btw, you need to add this to the table in https://github.com/llvm/llvm-project/blob/189033e6bede96de0d74e61715fcd1b48d95e247/clang/docs/LanguageExtensions.rst?plain=1#L1429 because this is an extension we intentionally support in older language modes (we want to let folks like libc++ rely on this behavior in Clang).

clang/lib/AST/Decl.cpp
1115–1122
clang/lib/Sema/SemaDecl.cpp
1979–1980

This could take const LangOptions & instead of const Sema & as we don't actually use Sema here.

aaron.ballman added inline comments.Jun 29 2023, 8:15 AM
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
4

Can you also add tests for global namespace and anonymous namespaces?

Can you add coverage for:

int _ = 12;
auto lam = [_ = 0]{}; // gets a placeholder extension warning (I don't think we issue a shadow warning currently, but probably should?)

and

void func() {
  // Is a placeholder variable and so it does not get a -Wunused-but-set warning despite
  int _ = 12;
  int x = 12; // Gets -Wunused-but-set warning
}
20

I think we want additional comments explaining why there's only three instances of this warning instead of four because this situation is kind of weird. The first declaration *is* a placeholder, so we suppress unused variable warnings for it. However, because a single variable named _ is valid code in earlier language modes, it's not really an *extension* until we introduce the second variable named _ in the same scope in terms of conformance. It doesn't help users to issue an extension warning on the first declaration because the diagnostic behavior change is more likely appreciated than a surprising change.

I think this information should be documented somewhere (perhaps just in the release notes with an example since we don't have much to document about the extension itself).

53
64–71

Can you add a test for this case:

struct WhoWritesCodeLikeThis {
  int _;
  void f() {
    int _;
    _ = 12; // Not ambiguous reference to placeholder, accesses the local variable which shadows the field
    (void)({ int _ = 12; _;}); // Also not an ambiguous reference to a placeholder, accesses the GNU statement expression variable which shadows the local variable
  }
  // None of the _ declarations should get the extension warning
};

and for:

typedef int _;
typedef int _; // Not a placeholder, but also not a redefinition, so this_is_fine.gif

and for:

struct S {
  int _, _;
};
constexpr int oh_no = __builtin_offsetof(S, _); // Ambiguous reference error
int S::* ref = &S::_; // Ambiguous reference error

and for:

struct S {
  int _, _;
  void func() __attribute__((diagnose_if(_ != 0, "oh no!", "warning")));
};
cor3ntin updated this revision to Diff 535846.Jun 29 2023, 9:22 AM
cor3ntin marked 5 inline comments as done.
  • Address Aaron's feedback
    • Add more tests
    • Add a longer description of the feature in the release notes
    • address nitpicks
  • Improve error message when using __builtin_offsetof

CCing David and Eric -- do you see any concerns regarding debug information for these changes?

Some tests with lldb

(lldb) b main
Breakpoint 1: where = placeholder`main + 4 at debug_placeholder.cpp:2:9, address = 0x0000555555555134
(lldb) run
Process 4044833 launched: '/home/cor3ntin/dev/compilers/LLVM/build-release/placeholder' (x86_64)
Process 4044833 stopped
* thread #1, name = 'placeholder', stop reason = breakpoint 1.1
    frame #0: 0x0000555555555134 placeholder`main at debug_placeholder.cpp:2:9
   1    int main() {
-> 2        int _ = 0;
   3        int _ = 0;
   4        int _ = 0;
   5        {
   6            int _ = 0;
   7            int _ = 0;
(lldb) step
Process 4044833 stopped
* thread #1, name = 'placeholder', stop reason = step in
    frame #0: 0x000055555555513b placeholder`main at debug_placeholder.cpp:3:9
   1    int main() {
   2        int _ = 0;
-> 3        int _ = 0;
   4        int _ = 0;
   5        {
   6            int _ = 0;
   7            int _ = 0;
(lldb) step
Process 4044833 stopped
* thread #1, name = 'placeholder', stop reason = step in
    frame #0: 0x0000555555555142 placeholder`main at debug_placeholder.cpp:4:9
   1    int main() {
   2        int _ = 0;
   3        int _ = 0;
-> 4        int _ = 0;
   5        {
   6            int _ = 0;
   7            int _ = 0;
(lldb) frame variable
(int) _ = 0
(int) _ = 0
(int) _ = 0
(lldb) step
Process 4044833 stopped
* thread #1, name = 'placeholder', stop reason = step in
    frame #0: 0x0000555555555149 placeholder`main at debug_placeholder.cpp:6:13
   3        int _ = 0;
   4        int _ = 0;
   5        {
-> 6            int _ = 0;
   7            int _ = 0;
   8        }
   9    }
(lldb) step
Process 4044833 stopped
* thread #1, name = 'placeholder', stop reason = step in
    frame #0: 0x0000555555555150 placeholder`main at debug_placeholder.cpp:7:13
   4        int _ = 0;
   5        {
   6            int _ = 0;
-> 7            int _ = 0;
   8        }
   9    }
(lldb) frame variable
(int) _ = 0
(int) _ = 0
(int) _ = 0
(int) _ = 0
(int) _ = 32767
(lldb)

Seems to work well enough @hubert.reinterpretcast

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

Can we have tests for:

struct { int _, _; } a = { ._ = 0 };

and

struct A {
  A();
  int _, _;
};

A::A() : _(0) {}

Maybe try testing with more different values, to demonstrate which entities the debugger is finding?

int _ = 1;
int _ = 2;
{
  int _ = 3;
  int _ = 7;
}

Or something like that. But, honestly, if the point is that these variables should be unnameable - perhaps we shouldn't generate any DWARF for them?

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

Codegen test for

static union { int _ = 42; };
int &ref = _;
int foo() { return 13; }
static union { int _ = foo(); };
int main(void) { return ref; }

might be interesting.

I suspect that this case was missed in the committee discussion of the paper @cor3ntin.

3

Less controversial tests to consider:

struct A {
  int _;
  union { int _; };
};
struct B { union { int _, _; }; };
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

In a similar vein, a codegen test for:

struct A { A(); };
inline void f [[gnu::used]]() {
  static union { A _{}; };
  static union { A _{}; };
}

Perhaps not intended to be allowed though (premise was no symbols with "linkage"?)

cor3ntin added inline comments.Jul 5 2023, 2:30 PM
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

What's interesting about

static union { int _ = 42; };
int &ref = _;
int foo() { return 13; }
static union { int _ = foo(); };
int main(void) { return ref; }

?
It's already supported by clang https://godbolt.org/z/6j89EdnEo

I'm adding the other tests (and fixing the associated bugs, of which there were a few...)

3

Perhaps not intended to be allowed though (premise was no symbols with "linkage"?)

Yes, this should be ill-formed, anything where we would have to mangle multiple _ should be ill-formed.
I do believe that's covered though, _ does not have storage duration.

Maybe try testing with more different values, to demonstrate which entities the debugger is finding?

int _ = 1;
int _ = 2;
{
  int _ = 3;
  int _ = 7;
}

Or something like that. But, honestly, if the point is that these variables should be unnameable - perhaps we shouldn't generate any DWARF for them?

The variables should still be inspectable, ideally. It's true it makes no sense to be able to use them in expressions, and maybe if lldb use clang to evaluate expressions that would just work?

philnik added a subscriber: philnik.Jul 5 2023, 2:36 PM

Would it be possible to make __ also a placeholder, so standard libraries can use this before C++26? Or is _ already a reserved identifier? (I don't think so)

Maybe try testing with more different values, to demonstrate which entities the debugger is finding?

int _ = 1;
int _ = 2;
{
  int _ = 3;
  int _ = 7;
}

Or something like that. But, honestly, if the point is that these variables should be unnameable - perhaps we shouldn't generate any DWARF for them?

The variables should still be inspectable, ideally. It's true it makes no sense to be able to use them in expressions, and maybe if lldb use clang to evaluate expressions that would just work?

I think that's going to be tricky - because we don't emit DWARF to say where one variable's lifetime starts compared to another in the same scope - so likely a debugger would just always show the first or last variable with the same name in a given scope (so in the above example you could probably only print {1,3} or {2,7}) - and getting it wrong is more likely and more problematic than existing cases of shadowed variables. If we can't get this right, which I don't think we can, practically speaking, at the moment (the DWARF feature we'd have to support to do this better would probably be DW_AT_start_scope to say at what instruction the lifetime of a variable starts) - it may be best not to include it at all?

Maybe try testing with more different values, to demonstrate which entities the debugger is finding?

int _ = 1;
int _ = 2;
{
  int _ = 3;
  int _ = 7;
}

Or something like that. But, honestly, if the point is that these variables should be unnameable - perhaps we shouldn't generate any DWARF for them?

The variables should still be inspectable, ideally. It's true it makes no sense to be able to use them in expressions, and maybe if lldb use clang to evaluate expressions that would just work?

I think that's going to be tricky - because we don't emit DWARF to say where one variable's lifetime starts compared to another in the same scope - so likely a debugger would just always show the first or last variable with the same name in a given scope (so in the above example you could probably only print {1,3} or {2,7}) - and getting it wrong is more likely and more problematic than existing cases of shadowed variables. If we can't get this right, which I don't think we can, practically speaking, at the moment (the DWARF feature we'd have to support to do this better would probably be DW_AT_start_scope to say at what instruction the lifetime of a variable starts) - it may be best not to include it at all?

It doesn't seems to be confused on simple examples

cpp
* thread #1, name = 'placeholder', stop reason = step in
    frame #0: 0x0000555555555157 placeholder`main at debug_placeholder.cpp:8:13
   5        {
   6            int _ = 4;
   7            int _ = 5;
-> 8            int _ = 6;
   9        }
   10   }
(lldb) frame variable
(int) _ = 1
(int) _ = 2
(int) _ = 3
(int) _ = 4
(int) _ = 5
(int) _ = -9432

That being said, if you think it's more prudent, I would not mind

Would it be possible to make __ also a placeholder, so standard libraries can use this before C++26? Or is _ already a reserved identifier? (I don't think so)

I don't see an issue allowing that in the std namespace. If we allow it everywhere, I'm afraid people will rely on it, which would hurt portability/be confusing.

cor3ntin added inline comments.Jul 5 2023, 2:59 PM
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

What's interesting about

static union { int _ = 42; };
int &ref = _;
int foo() { return 13; }
static union { int _ = foo(); };
int main(void) { return ref; }

?
It's already supported by clang https://godbolt.org/z/6j89EdnEo

I'm adding the other tests (and fixing the associated bugs, of which there were a few...)

I see it now. Thanks, I hate it. There is apparently a preexisting bug.
And yes, i think we should say something about members of anonymous union declared at namespace scope in the standard.... I realize now this is missing
Thanks for catching that.

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

Thanks for catching that.

Glad to be of help!

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

I do believe that's covered though, _ does not have storage duration.

It's not covered by the wording: _ is a non-static data member.

Maybe try testing with more different values, to demonstrate which entities the debugger is finding?

int _ = 1;
int _ = 2;
{
  int _ = 3;
  int _ = 7;
}

Or something like that. But, honestly, if the point is that these variables should be unnameable - perhaps we shouldn't generate any DWARF for them?

The variables should still be inspectable, ideally. It's true it makes no sense to be able to use them in expressions, and maybe if lldb use clang to evaluate expressions that would just work?

I think that's going to be tricky - because we don't emit DWARF to say where one variable's lifetime starts compared to another in the same scope - so likely a debugger would just always show the first or last variable with the same name in a given scope (so in the above example you could probably only print {1,3} or {2,7}) - and getting it wrong is more likely and more problematic than existing cases of shadowed variables. If we can't get this right, which I don't think we can, practically speaking, at the moment (the DWARF feature we'd have to support to do this better would probably be DW_AT_start_scope to say at what instruction the lifetime of a variable starts) - it may be best not to include it at all?

It doesn't seems to be confused on simple examples

cpp
* thread #1, name = 'placeholder', stop reason = step in
    frame #0: 0x0000555555555157 placeholder`main at debug_placeholder.cpp:8:13
   5        {
   6            int _ = 4;
   7            int _ = 5;
-> 8            int _ = 6;
   9        }
   10   }
(lldb) frame variable
(int) _ = 1
(int) _ = 2
(int) _ = 3
(int) _ = 4
(int) _ = 5
(int) _ = -9432

That being said, if you think it's more prudent, I would not mind

Huh, I guess maybe it's got some heuristics based on line number. Could probably thwart those by stamping this out with a macro (which would cause all code in the macro to be attributed to the line where the macro is named) - add some function calls in, so you can step back up from the call to get to more precise locations and you'd probably see some problems. That's probably a bit contrived (but maybe not - perhaps _ would be more common in macros).

*shrug* maybe it's OK enough.

cor3ntin updated this revision to Diff 537742.Jul 6 2023, 8:29 AM

Address Hubert's feedback

  • Add code and tests to properly support member initializer
  • Add code and tests to support designated initializers
  • Correctly hamdle members of anonymous union
  • Make placeholder redeclaration in anonymous static union ill-formed, even if the wording fails to handle that case.
cor3ntin updated this revision to Diff 537745.Jul 6 2023, 8:34 AM

Add tests for static structured bindings

Seems to work well enough @hubert.reinterpretcast

It seems the class member case trips up debuggers.

union U {
  struct A { int _, _; } a;
  struct B { int x, y; } b;
};
U u = {{1, 2}};

int main(void) { return u.b.x; }
(lldb) b main
Breakpoint 1: where = a.out`main + 28 at placeholderDbg.cc:7:18, address = 0x000000000001080c
(lldb) r
Process 2339034 launched: '/terrannew/hstong/.Lpcoral03/llvmbld/a.out' (powerpc64le)
Process 2339034 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x000000010001080c a.out`main at placeholderDbg.cc:7:18
   4    };
   5    U u = {{1, 2}};
   6
-> 7    int main(void) { return u.b.x; }
(lldb) print u
(U) {
  a = (_ = 1)
  b = (x = 1, y = 2)
}
(lldb)

It seems the class member case trips up debuggers.

If llvm-dwarfdump is to be believed, it looks like a compiler bug (two members reported at the same offset).

0x000000a3:       DW_TAG_member [4]   (0x0000009d)
                    DW_AT_name [DW_FORM_string] ("_")
                    DW_AT_type [DW_FORM_ref4]   (cu + 0x00de => {0x000000de} "int")
                    DW_AT_decl_file [DW_FORM_data1]     ("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg.cc")
                    DW_AT_decl_line [DW_FORM_data1]     (2)
                    DW_AT_data_member_location [DW_FORM_data1]  (0x00)

0x000000ad:       DW_TAG_member [4]   (0x0000009d)
                    DW_AT_name [DW_FORM_string] ("_")
                    DW_AT_type [DW_FORM_ref4]   (cu + 0x00de => {0x000000de} "int")
                    DW_AT_decl_file [DW_FORM_data1]     ("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg.cc")
                    DW_AT_decl_line [DW_FORM_data1]     (2)
                    DW_AT_data_member_location [DW_FORM_data1]  (0x00)
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

@cor3ntin, it seems the designated initializer case has not been added.

46

Just noting: This case is allowed by the wording, and I suspect it requires extended discussion.

If llvm-dwarfdump is to be believed, it looks like a compiler bug (two members reported at the same offset).

Similarly, only one local variable out of two in the same line reported:

$ cat placeholderDbg2.cc
extern "C" int printf(const char *, ...);
struct A {
  A(int x) : x(x) { }
  ~A() { printf("%d\n", x); }
  int x;
};
int main() {
  A _ = 0, _ = 1;
  return 0;
}
Return:  0x00:0   Sat Jul  8 23:52:46 2023 EDT
(lldb) target create "a.out"
Current executable set to '/terrannew/hstong/.Lpcoral03/llvmbld/a.out' (powerpc64le).
(lldb) b 9
Breakpoint 1: where = a.out`main + 68 at placeholderDbg2.cc:9:3, address = 0x0000000000010ab4
(lldb) r
Process 2951717 launched: '/terrannew/hstong/.Lpcoral03/llvmbld/a.out' (powerpc64le)
Process 2951717 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x0000000100010ab4 a.out`main at placeholderDbg2.cc:9:3
   6    };
   7    int main() {
   8      A _ = 0, _ = 1;
-> 9      return 0;
   10   }
(lldb) var
(A) _ = (x = 0)
(lldb) c
Process 2951717 resuming
1
0
Process 2951717 exited with status = 0 (0x00000000)
(lldb) q
0x0000009b:   DW_TAG_subprogram [14] * (0x0000000b)
                DW_AT_low_pc [DW_FORM_addr]     (0x0000000000010a70)
                DW_AT_high_pc [DW_FORM_data4]   (0x000000b4)
                DW_AT_frame_base [DW_FORM_exprloc]      (DW_OP_reg31 X31)
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x00000086] = "main")
                DW_AT_decl_file [DW_FORM_data1] ("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg2.cc")
                DW_AT_decl_line [DW_FORM_data1] (7)
                DW_AT_type [DW_FORM_ref4]       (cu + 0x008f => {0x0000008f} "int")
                DW_AT_external [DW_FORM_flag_present]   (true)

0x000000b4:     DW_TAG_variable [15]   (0x0000009b)
                  DW_AT_location [DW_FORM_exprloc]      (DW_OP_fbreg +128)
                  DW_AT_name [DW_FORM_strp]     ( .debug_str[0x0000008b] = "_")
                  DW_AT_decl_file [DW_FORM_data1]       ("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg2.cc")
                  DW_AT_decl_line [DW_FORM_data1]       (8)
                  DW_AT_type [DW_FORM_ref4]     (cu + 0x005a => {0x0000005a} "A")

0x000000c3:     NULL
cor3ntin updated this revision to Diff 538545.EditedJul 10 2023, 2:03 AM

Add more lookup tests

cor3ntin updated this revision to Diff 538549.Jul 10 2023, 2:17 AM

Add missing designated initializer test

Similarly, only one local variable out of two in the same line reported:

I can confirm that adding a lexical block scope causes both variables to be recorded into the debug info (thus this is a debug info generation issue unique to the new feature).

@dblaikie Would you be willing to look at the debugger side of things in a subsequent patch? I'm not familiar with debug symbol code gen so I'm not sure I'd be able to improve thing the right way.

clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
3

Thanks for noticing!

clang/docs/ReleaseNotes.rst
136–139

Use double backticks to start/end inline code in RST.

clang/lib/Sema/SemaInit.cpp
2607

Needs rebase on top of 632dd6a4ca00.

@dblaikie Would you be willing to look at the debugger side of things in a subsequent patch? I'm not familiar with debug symbol code gen so I'm not sure I'd be able to improve thing the right way.

Not sure I've got the time to do the fix myself, but might be able to provide pointers.

but at least at a first blush I can't reproduce the failures shown...

struct t1 {
  int _;
  int _;
};
t1 v1;
int main() {
  int _;
  int _;
}
0x0000002e:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("t1")
                DW_AT_byte_size (0x08)
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                DW_AT_decl_line (1)

0x00000034:     DW_TAG_member
                  DW_AT_name    ("_")
                  DW_AT_type    (0x00000047 "int")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (2)
                  DW_AT_data_member_location    (0x00)

0x0000003d:     DW_TAG_member
                  DW_AT_name    ("_")
                  DW_AT_type    (0x00000047 "int")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (2)
                  DW_AT_data_member_location    (0x00)

0x00000046:     NULL

0x00000047:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x0000004b:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000008)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                DW_AT_decl_line (6)
                DW_AT_type      (0x00000047 "int")
                DW_AT_external  (true)

0x0000005a:     DW_TAG_variable
                  DW_AT_location        (DW_OP_fbreg -4)
                  DW_AT_name    ("_")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (7)
                  DW_AT_type    (0x00000047 "int")

0x00000065:     DW_TAG_variable
                  DW_AT_location        (DW_OP_fbreg -8)
                  DW_AT_name    ("_")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (8)
                  DW_AT_type    (0x00000047 "int")

Looks OK to me - two local variables with the same name, two member variables with the same name.

so probably at least one bug in lldb because it does seem to think t1 has only one member. But the DWARF I see for the local variables doesn't seem to match the dump shown in https://reviews.llvm.org/D153536#4483191

FYI, my current aim is to land that early in the Clang 18 cycle, that should give us time to find any remaining issue and maybe improve debugger support!

but at least at a first blush I can't reproduce the failures shown...

@dblaikie, you did reproduce the issue with the members. Both entries have DW_AT_decl_line 2 and DW_AT_data_member_location 0 (the second entry should indicate DW_AT_decl_line 3 and DW_AT_data_member_location 4):

0x0000002e:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("t1")
                DW_AT_byte_size (0x08)
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                DW_AT_decl_line (1)

0x00000034:     DW_TAG_member
                  DW_AT_name    ("_")
                  DW_AT_type    (0x00000047 "int")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (2)
                  DW_AT_data_member_location    (0x00)

0x0000003d:     DW_TAG_member
                  DW_AT_name    ("_")
                  DW_AT_type    (0x00000047 "int")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (2)
                  DW_AT_data_member_location    (0x00)

0x00000046:     NULL

As for the block-scope case, I am still able to reproduce the issue (and also your result that does not exhibit the issue). The key seems to be having the _s on the same line.

but at least at a first blush I can't reproduce the failures shown...

@dblaikie, you did reproduce the issue with the members. Both entries have DW_AT_decl_line 2 and DW_AT_data_member_location 0 (the second entry should indicate DW_AT_decl_line 3 and DW_AT_data_member_location 4):

0x0000002e:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("t1")
                DW_AT_byte_size (0x08)
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                DW_AT_decl_line (1)

0x00000034:     DW_TAG_member
                  DW_AT_name    ("_")
                  DW_AT_type    (0x00000047 "int")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (2)
                  DW_AT_data_member_location    (0x00)

0x0000003d:     DW_TAG_member
                  DW_AT_name    ("_")
                  DW_AT_type    (0x00000047 "int")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
                  DW_AT_decl_line       (2)
                  DW_AT_data_member_location    (0x00)

0x00000046:     NULL

As for the block-scope case, I am still able to reproduce the issue (and also your result that does not exhibit the issue). The key seems to be having the _s on the same line.

Ah, fair enough - probably easy enough for you to look for some kind of map with inadequate key material in CGDebugInfo somewhere, I'd guess...

From yesterday's C++ clang WG meeting

  • We can land this now
  • We need to track the debugger issue
  • We should mention the lack of debugging support in the release notes
  • We should strive to improve the situation in the 18 time frame but it would not be blocker as few people may be impacted.
  • We should remember to modify the release notes when debugger support is improved if that happens before clang 18

From yesterday's C++ clang WG meeting

  • We can land this now
  • We need to track the debugger issue
  • We should mention the lack of debugging support in the release notes
  • We should strive to improve the situation in the 18 time frame but it would not be blocker as few people may be impacted.
  • We should remember to modify the release notes when debugger support is improved if that happens before clang 18

Agreed; there are some minor things that need to be tweaked before landing this now, but I think the patch is pretty close to finished.

clang/lib/Sema/SemaDeclCXX.cpp
4332
4343
4355
clang/www/cxx_status.html
148
cor3ntin updated this revision to Diff 546910.Aug 3 2023, 9:53 AM
cor3ntin marked an inline comment as done.
  • Rebase
  • Address Aaron's comments
  • Mention the lack of debugging support in the release notes.
cor3ntin marked 10 inline comments as done.Aug 3 2023, 10:01 AM
cor3ntin updated this revision to Diff 546915.Aug 3 2023, 10:02 AM

Fix release notes formatting

aaron.ballman accepted this revision.Aug 4 2023, 5:44 AM

LGTM aside from some minor tweaks (some extra parens to remove, a spurious comment to remove).

clang/lib/Sema/SemaDeclCXX.cpp
4332
4343
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
2–3
This revision is now accepted and ready to land.Aug 4 2023, 5:44 AM
cor3ntin updated this revision to Diff 547206.Aug 4 2023, 7:33 AM
cor3ntin marked an inline comment as done.

Fix nits

This revision was landed with ongoing or failed builds.Aug 4 2023, 7:51 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka added a subscriber: vitalybuka.

This patch brakes https://lab.llvm.org/buildbot/#/builders/168/builds/14997
Not sure what is wrong there, probably the test need to be updated. Can you please take a look?

This patch brakes https://lab.llvm.org/buildbot/#/builders/168/builds/14997
Not sure what is wrong there, probably the test need to be updated. Can you please take a look?

I'm a bit confused -- the linked bot is green and the failures to either side of the linked run are both failing for the same reason (which seems to be unrelated to the changes in this patch). It looks like this bot went red here: https://lab.llvm.org/buildbot/#/builders/168/builds/14944 and hasn't reliably come back to green.

This patch brakes https://lab.llvm.org/buildbot/#/builders/168/builds/14997
Not sure what is wrong there, probably the test need to be updated. Can you please take a look?

I'm a bit confused -- the linked bot is green and the failures to either side of the linked run are both failing for the same reason (which seems to be unrelated to the changes in this patch). It looks like this bot went red here: https://lab.llvm.org/buildbot/#/builders/168/builds/14944 and hasn't reliably come back to green.

Sorry copy-pasted wrong url. This green is manual request on revision before this patch.

Correct failure https://lab.llvm.org/buildbot/#/builders/168/builds/14944
So it's persistently red after that.

If anyone wondering, I've bisected to this patch on buildbot host.
For whatever reason does not reproduce on my workstation.

Looking at the failing test (clang-tools-extra/clangd/test/crash-preamble.test), I'd be very surprising if it is related to a front end change like this PR. Any idea @sammccall ?

Not a test update

The second line of the test triggers:

RUN: not --crash clangd -lit-test -sync=0 < %s 2> %t.async.err

Signalled while building preamble
  Filename: =================================================================
==2319884==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f310f6eb210 at pc 0x55a3ecaab9f5 bp 0x7f3115c49920 sp 0x7f3115c49918
READ of size 1 at 0x7f310f6eb210 thread T3
    #0 0x55a3ecaab9f4 in __is_long /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/string:1734:33
    #1 0x55a3ecaab9f4 in __get_pointer /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/string:1869:17
    #2 0x55a3ecaab9f4 in data /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/string:1559:73
    #3 0x55a3ecaab9f4 in operator<< /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/raw_ostream.h:249:22
    #4 0x55a3ecaab9f4 in clang::clangd::(anonymous namespace)::crashDumpCompileCommand(llvm::raw_ostream&, clang::tooling::CompileCommand const&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:1012:24
    #5 0x55a3ecaab285 in clang::clangd::(anonymous namespace)::crashDumpParseInputs(llvm::raw_ostream&, clang::clangd::ParseInputs const&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:1038:3
    #6 0x55a3ecec1c9d in operator() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:382:12
    #7 0x55a3ecec1c9d in clang::clangd::ThreadCrashReporter::runCrashHandlers() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/ThreadCrashReporter.cpp:30:5
    #8 0x55a3ebacfd78 in operator() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:717:9
    #9 0x55a3ebacfd78 in clang::clangd::clangdMain(int, char**)::$_0::__invoke(void*) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:716:7
    #10 0x55a3e87c27ba in llvm::sys::RunSignalHandlers() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/Signals.cpp:103:5
    #11 0x55a3e87cbd01 in SignalHandler(int) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/Unix/Signals.inc:403:3
    #12 0x7f3115a3bcef  (/lib/x86_64-linux-gnu/libc.so.6+0x3bcef) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #13 0x7f3115a9226a in pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x9226a) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #14 0x7f3115a3bc45 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x3bc45) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #15 0x7f3115a227fb in abort (/lib/x86_64-linux-gnu/libc.so.6+0x227fb) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #16 0x55a3e869c429 in llvm::report_fatal_error(llvm::Twine const&, bool) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ErrorHandling.cpp:123:5
    #17 0x55a3e869c014 in llvm::report_fatal_error(char const*, bool) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ErrorHandling.cpp:83:3
    #18 0x55a3e9a2bb12 in (anonymous namespace)::PragmaDebugHandler::HandlePragma(clang::Preprocessor&, clang::PragmaIntroducer, clang::Token&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Lex/Pragma.cpp:1102:9
    #19 0x55a3e9a0f270 in clang::Preprocessor::HandlePragmaDirective(clang::PragmaIntroducer) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Lex/Pragma.cpp:178:19
    #20 0x55a3e98e92a1 in clang::Preprocessor::HandleDirective(clang::Token&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Lex/PPDirectives.cpp:1265:14
    #21 0x55a3e98a427a in clang::Lexer::LexTokenInternal(clang::Token&, bool) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Lex/Lexer.cpp:4362:7
    #22 0x55a3e989754c in clang::Lexer::Lex(clang::Token&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Lex/Lexer.cpp:3578:24
    #23 0x55a3e9a5fb72 in clang::Preprocessor::Lex(clang::Token&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Lex/Preprocessor.cpp:886:33
    #24 0x55a3eee8d7ef in ConsumeToken /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/include/clang/Parse/Parser.h:504:8
    #25 0x55a3eee8d7ef in clang::Parser::Initialize() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:568:3
    #26 0x55a3eee7732f in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseAST.cpp:156:7
    #27 0x55a3ec4ddff4 in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1063:8
    #28 0x55a3ec2320b6 in clang::PrecompiledPreamble::Build(clang::CompilerInvocation const&, llvm::MemoryBuffer const*, clang::PreambleBounds, clang::DiagnosticsEngine&, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::__1::shared_ptr<clang::PCHContainerOperations>, bool, llvm::StringRef, clang::PreambleCallbacks&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang/lib/Frontend/PrecompiledPreamble.cpp:535:30
    #29 0x55a3ec9094b9 in clang::clangd::buildPreamble(llvm::StringRef, clang::CompilerInvocation, clang::clangd::ParseInputs const&, bool, std::__1::function<void (clang::clangd::CapturedASTCtx, std::__1::shared_ptr<clang::include_cleaner::PragmaIncludes const>)>, clang::clangd::PreambleBuildStats*) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/Preamble.cpp:663:24
    #30 0x55a3ecaaf64c in clang::clangd::(anonymous namespace)::PreambleThread::build(clang::clangd::(anonymous namespace)::PreambleThread::Request) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:1080:17
    #31 0x55a3ecaad02b in run /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:505:9
    #32 0x55a3ecaad02b in operator() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:819:55
    #33 0x55a3ecaad02b in void llvm::detail::UniqueFunctionBase<void>::CallImpl<clang::clangd::(anonymous namespace)::ASTWorker::create(llvm::StringRef, clang::clangd::GlobalCompilationDatabase const&, clang::clangd::TUScheduler::ASTCache&, clang::clangd::TUScheduler::HeaderIncluderCache&, clang::clangd::AsyncTaskRunner*, clang::clangd::Semaphore&, clang::clangd::TUScheduler::Options const&, clang::clangd::ParsingCallbacks&)::$_1>(void*) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:220:12
    #34 0x55a3ecec3715 in operator() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:382:12
    #35 0x55a3ecec3715 in operator() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:101:5
    #36 0x55a3ecec3715 in operator()<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:98:15) &> /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:43:11
    #37 0x55a3ecec3715 in __invoke<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:42:9), (lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:98:15) &> /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/__type_traits/invoke.h:340:25
    #38 0x55a3ecec3715 in __apply_tuple_impl<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:42:9), std::__1::tuple<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:98:15)> &, 0UL> /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/tuple:1825:1
    #39 0x55a3ecec3715 in apply<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:42:9), std::__1::tuple<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:98:15)> &> /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/tuple:1834:1
    #40 0x55a3ecec3715 in GenericThreadProxy<std::__1::tuple<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:98:15)> > /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:41:5
    #41 0x55a3ecec3715 in void* llvm::thread::ThreadProxy<std::__1::tuple<clang::clangd::AsyncTaskRunner::runAsync(llvm::Twine const&, llvm::unique_function<void ()>)::$_1>>(void*) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:55:5
    #42 0x55a3e8393668 in asan_thread_start(void*) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:234:28
    #43 0x7f3115a90401  (/lib/x86_64-linux-gnu/libc.so.6+0x90401) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #44 0x7f3115b1f58f  (/lib/x86_64-linux-gnu/libc.so.6+0x11f58f) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)

Address 0x7f310f6eb210 is located in stack of thread T3 at offset 528 in frame
    #0 0x55a3efe9b3ff in llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<(anonymous namespace)::DefaultAllocator>, (anonymous namespace)::DefaultAllocator>::parseType() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Demangle/ItaniumDemangle.h:3772

  This frame has 20 object(s):
    [32, 40) 'ClassType.i' (line 3668)
    [64, 72) 'MemberType.i' (line 3671)
    [96, 104) 'Dimension.i' (line 3640)
    [128, 144) 'ref.tmp.i' (line 3643)
    [160, 168) 'Ty.i' (line 3657)
    [192, 200) 'Result' (line 3773)
    [224, 240) 'Res' (line 3886)
    [256, 264) 'DimensionNumber' (line 3916)
    [288, 304) 'ref.tmp' (line 3916)
    [320, 328) 'Size' (line 3931)
    [352, 368) 'ref.tmp179' (line 3931)
    [384, 392) 'Child' (line 3978)
    [416, 424) 'TA' (line 4032)
    [448, 456) 'Ptr' (line 4042)
    [480, 488) 'Ref' (line 4051)
    [512, 520) 'Ref309' (line 4060) <== Memory access at offset 528 overflows this variable
    [544, 552) 'P' (line 4069)
    [576, 584) 'P333' (line 4078)
    [608, 609) 'IsSubst' (line 4087)
    [624, 632) 'TA364' (line 4105)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
Thread T3 created by T0 here:
    #0 0x55a3e837bdaf in pthread_create /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:245:3
    #1 0x55a3e87ce1c4 in llvm::llvm_execute_on_thread_impl(void* (*)(void*), void*, std::__1::optional<unsigned int>) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/Unix/Threading.inc:85:17
    #2 0x55a3ecec2e1d in thread<(lambda at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:98:15)> /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/thread.h:131:12
    #3 0x55a3ecec2e1d in clang::clangd::AsyncTaskRunner::runAsync(llvm::Twine const&, llvm::unique_function<void ()>) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/support/Threading.cpp:107:16
    #4 0x55a3eca955c5 in create /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:818:12
    #5 0x55a3eca955c5 in clang::clangd::TUScheduler::update(llvm::StringRef, clang::clangd::ParseInputs, clang::clangd::WantDiagnostics) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/TUScheduler.cpp:1671:30
    #6 0x55a3ec160093 in clang::clangd::ClangdServer::addDocument(llvm::StringRef, llvm::StringRef, llvm::StringRef, clang::clangd::WantDiagnostics, bool) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/ClangdServer.cpp:303:33
    #7 0x55a3ec0447d6 in clang::clangd::ClangdLSPServer::onDocumentDidOpen(clang::clangd::DidOpenTextDocumentParams const&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp:701:11
    #8 0x55a3ec111568 in void clang::clangd::LSPBinder::notification<clang::clangd::DidOpenTextDocumentParams, clang::clangd::ClangdLSPServer>(llvm::StringLiteral, clang::clangd::ClangdLSPServer*, void (clang::clangd::ClangdLSPServer::*)(clang::clangd::DidOpenTextDocumentParams const&))::'lambda'(llvm::json::Value)::operator()(llvm::json::Value) const /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/LSPBinder.h:153:5
    #9 0x55a3ec1112fc in void llvm::detail::UniqueFunctionBase<void, llvm::json::Value>::CallImpl<void clang::clangd::LSPBinder::notification<clang::clangd::DidOpenTextDocumentParams, clang::clangd::ClangdLSPServer>(llvm::StringLiteral, clang::clangd::ClangdLSPServer*, void (clang::clangd::ClangdLSPServer::*)(clang::clangd::DidOpenTextDocumentParams const&))::'lambda'(llvm::json::Value)>(void*, llvm::json::Value&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:220:12
    #10 0x55a3ec08f34b in operator() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:382:12
    #11 0x55a3ec08f34b in clang::clangd::ClangdLSPServer::MessageHandler::onNotify(llvm::StringRef, llvm::json::Value) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp:200:7
    #12 0x55a3ec852a14 in handleMessage /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/JSONTransport.cpp:196:18
    #13 0x55a3ec852a14 in clang::clangd::(anonymous namespace)::JSONTransport::loop(clang::clangd::Transport::MessageHandler&) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/JSONTransport.cpp:120:16
    #14 0x55a3ec079073 in clang::clangd::ClangdLSPServer::run() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp:1693:25
    #15 0x55a3ebabacd7 in clang::clangd::clangdMain(int, char**) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:1006:28
    #16 0x7f3115a2350f  (/lib/x86_64-linux-gnu/libc.so.6+0x2350f) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)

SUMMARY: AddressSanitizer: stack-use-after-return /b/sanitizer-x86_64-linux-bootstrap-asan/build/libcxx_build_asan/include/c++/v1/string:1734:33 in __is_long
Shadow bytes around the buggy address:
  0x7f310f6eaf80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f310f6eb000: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb080: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb100: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb180: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
=>0x7f310f6eb200: f5 f5[f5]f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb280: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb300: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb380: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb400: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7f310f6eb480: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==2319884==ABORTING

This patch brakes https://lab.llvm.org/buildbot/#/builders/168/builds/14997
Not sure what is wrong there, probably the test need to be updated. Can you please take a look?

I'm a bit confused -- the linked bot is green and the failures to either side of the linked run are both failing for the same reason (which seems to be unrelated to the changes in this patch). It looks like this bot went red here: https://lab.llvm.org/buildbot/#/builders/168/builds/14944 and hasn't reliably come back to green.

Sorry copy-pasted wrong url. This green is manual request on revision before this patch.

Correct failure https://lab.llvm.org/buildbot/#/builders/168/builds/14944
So it's persistently red after that.

If anyone wondering, I've bisected to this patch on buildbot host.
For whatever reason does not reproduce on my workstation.

Thanks! We might need someone from clangd to help here. The logs do not have any information explaining what's failing with the test, and looking at the test code, I struggle to see how these changes would impact that test. We've had some significant issues with flaky clangd tests (https://github.com/clangd/clangd/issues/1712), so it's possible this is happenstance.

Thanks! We might need someone from clangd to help here. The logs do not have any information explaining what's failing with the test, and looking at the test code, I struggle to see how these changes would impact that test. We've had some significant issues with flaky clangd tests (https://github.com/clangd/clangd/issues/1712), so it's possible this is happenstance.

I tried to debug that, and it looks like false Asan report. Maybe a bug in FakeStack::GC related code.

Thanks! We might need someone from clangd to help here. The logs do not have any information explaining what's failing with the test, and looking at the test code, I struggle to see how these changes would impact that test. We've had some significant issues with flaky clangd tests (https://github.com/clangd/clangd/issues/1712), so it's possible this is happenstance.

I tried to debug that, and it looks like false Asan report. Maybe a bug in FakeStack::GC related code.

Thank you for looking into it! I came to the same general conclusion; and it seems the impacted bot has gone back to green in the meantime: https://lab.llvm.org/buildbot/#/builders/168/builds/15031 so hopefully we're in good shape.

Thanks! We might need someone from clangd to help here. The logs do not have any information explaining what's failing with the test, and looking at the test code, I struggle to see how these changes would impact that test. We've had some significant issues with flaky clangd tests (https://github.com/clangd/clangd/issues/1712), so it's possible this is happenstance.

I tried to debug that, and it looks like false Asan report. Maybe a bug in FakeStack::GC related code.

Thank you for looking into it! I came to the same general conclusion; and it seems the impacted bot has gone back to green in the meantime: https://lab.llvm.org/buildbot/#/builders/168/builds/15031 so hopefully we're in good shape.

Yes. Fixed by D157552. It's amusing to see this unrelated patch exposing ~10 years old bug, which was never reported before.

Thanks! We might need someone from clangd to help here. The logs do not have any information explaining what's failing with the test, and looking at the test code, I struggle to see how these changes would impact that test. We've had some significant issues with flaky clangd tests (https://github.com/clangd/clangd/issues/1712), so it's possible this is happenstance.

I tried to debug that, and it looks like false Asan report. Maybe a bug in FakeStack::GC related code.

Thank you for looking into it! I came to the same general conclusion; and it seems the impacted bot has gone back to green in the meantime: https://lab.llvm.org/buildbot/#/builders/168/builds/15031 so hopefully we're in good shape.

Yes. Fixed by D157552. It's amusing to see this unrelated patch exposing ~10 years old bug, which was never reported before.

Oh wow! Great fix, thank you!