[Sema] Fix handling of functions that hide classes

[john.brawn]

Summary

When a function is declared in the same scope as a class with the same name then the function hides that class. Currently this is done by a single check after the main loop in LookupResult::resolveKind, but this can give the wrong result when we have a using declaration in multiple namespace scopes in two different ways:

• When the using declaration is hidden in one namespace but not the other we can end up considering only the hidden one when deciding if the result is ambiguous, causing an incorrect "not ambiguous" result.
• When two classes with the same name in different namespace scopes are both hidden by using declarations this can result in incorrectly deciding the result is ambiguous. There's currently a comment saying this is expected, but I don't think that's correct.

Solve this by checking each Decl to see if it's hidden by some other Decl in the same scope. This means we have to delay removing anything from Decls until after the main loop, in case a Decl is hidden by another that is removed due to being non-unique.

[rjmccall]

clang/lib/Sema/SemaLookup.cpp

542

This is going to fire on every single ordinary lookup that finds multiple declarations, right? I haven't fully internalized the issue you're solving here, but this is a very performance-sensitive path in the compiler; there's a reason this code is written to very carefully only do extra work when we've detected in the loop below that we're in a hidden-declarations situation. Is there any way we can restore that basic structure?

[john.brawn]

Avoid doing work when we don't have both decls that can hide and decls that can be hidden.

[john.brawn]

clang/lib/Sema/SemaLookup.cpp

542

Test4 in the added tests is an example of why we can't wait until after the main loop. The using A::X in namespace D is eliminated by the UniqueResult check, so the check for a declaration being hidden can only see the using declarations in namespace C. We also can't do it in the loop itself, as then we can't handle Test5: at the time we process the using A::X in namespace D it looks like it may cause ambiguity, but it's later hidden by the using B::X in the same namespace which we haven't yet processed.

I have adjusted it though so the nested loop and erasing of decls only happens when we both have things that hide and things that can be hidden. Doing some quick testing of compiling SemaOpenMP.cpp (the largest file in clang), LookupResult::resolveKind is called 75318 times, of which 75283 calls have HideTags=true, of which 56 meet this condition, i.e. 0.07%.

[john.brawn]

Same patch as previous, but with full context.

[rjmccall]

clang/lib/Sema/SemaLookup.cpp

542

Okay, I can see why you need to not mix tag-hiding with the removal of duplicates. However, I think you can maintain the current structure by delaying the actual removal of declarations until after the main loop; have the loop build up a set of indices to remove instead. (Also, you can keep this set as a bitset instead of a std::set<unsigned>.)

It's a shame that the hiding algorithm has to check every other declaration in the lookup in case they're from different scopes. I guess to avoid that we'd have to do the filtering immediately when we collect the declarations from a particular DC.

[shafik]

For more visibility.

[shafik]

clang/test/SemaCXX/using-hiding.cpp

21

I think namespace.udecl p10 disagrees, specifically:

using A::g;                           // error: conflicts with B​::​g

but I may be misreading CC @Endill who has been looking at p1787r6 in details where a lot of this wording changed.

[shafik]

clang/lib/Sema/SemaLookup.cpp

507

This section does not exist anymore, it was replaced in p1787 which resolved a very large number of DRs and reflector discussions. I have not fully digested the paper myself but this change should reflect the new wording as it exists in the current draft.

[john.brawn]

clang/lib/Sema/SemaLookup.cpp

507

It looks like https://eel.is/c++draft/basic.lookup#general-4 is the same thing but worded differently. That draft hasn't gone into a published standard though, and could change before it gets published, and the same section is referenced elsewhere in this file (and there are probably other references in this file to parts of the standard that will get changed in the next version), so I think it would make more sense to change all such comments at once when that change has gone into a published version of the standard.

clang/test/SemaCXX/using-hiding.cpp

21

I think that would mean that the using-declaration is ill-formed (i.e. we would give an error before we even arrived at looking up the name X). I'll try to adjust these tests so that they won't fail due to this when clang implements this behaviour.

[john.brawn]

clang/lib/Sema/SemaLookup.cpp

542

I think that delaying the removal until after the main loop would just complicate things, as then in the main loop we would have to check each index to see if it's something we're going to later remove. I can adjust it to do the erasing more like it's done in the main loop though, i.e. move the erased element to the end and decrement N, so the call to Decls.truncate will remove it. We can't use bitset though, as that takes the size of the bitset (which in this case would be the number of decls) as a template parameter.

[dexonsmith]

clang/lib/Sema/SemaLookup.cpp

542

llvm::BitVector should work for this.

[john.brawn]

Use BitVector, change how Decls are deleted, adjust test to avoid potentially-conflicting using-declarations.

[rjmccall]

clang/lib/Sema/SemaLookup.cpp

542

Why would the main loop need to check indices to see if it's something we're going to remove? You just need to check whether a tag is hidden before you add it to UniqueTypes.

[john.brawn]

clang/lib/Sema/SemaLookup.cpp

542

That seems to be the same thing I said, but phrased in a different way? It's not entirely clear to me what exactly you want the code to look like. It sound like you'd want it to be something like

llvm::BitVector HiddenDecls(N);
if (HideTags) {
  // Add hidden decls to HiddenDecls
}
while (I < N) {
  if (HiddenDecls[I]) // Ignore hidden decls
    continue;
  // Existing code in main loop
}
for (I : HiddenDecls)
  // remove this decl

I don't see how this would be better. Why delay the removal when we can just do it right away?

[rjmccall]

I'm suggesting:

llvm::BitVector declsToRemove;

while (I < N) {
  if (shouldHideTag()) declsToRemove.add(I);
  if (notUnique()) declsToRemove.add(I);
}

Decls.removeAll(declsToRemove)

[shafik]

clang/lib/Sema/SemaLookup.cpp

507

This paper was voted in 11/2020 and it considered C++23 and so it is safe to rely on this wording. Any behavior that does not match this wording should be considered a bug.

[john.brawn]

Restructured to check for hidden tags in the main loop. Also add a bunch of extra tests.

[rjmccall]

Thank you, this looks great, and I really appreciate that you found a way to make it work with just the single loop (in the typical case).

[john.brawn]

The first version of this that I committed caused a failure in clang/test/Modules/stress1.cpp so I reverted it. I've now committed a new version that handles the removal of existing decl when isPreferredLookupResult is true in a slightly different way, which should fix that failure.

[kadircet]

Hi!

After this patch clang seems to be crashing on:

class a {
  static c a;
  void b(){a};
};

with stack trace and assertion error:

$ ~/repos/llvm/build/bin/clang -xc++ repro.cc
repro.cc:2:10: error: unknown type name 'c'
    2 |   static c a;
      |          ^
repro.cc:2:12: error: member 'a' has the same name as its class
    2 |   static c a;
      |            ^
clang-17: /usr/local/google/home/kadircet/repos/llvm/clang/lib/Sema/SemaLookup.cpp:333: bool clang::LookupResult::checkDebugAssumptions() const: Assertion `ResultKind != Found || Decls.size() == 1' 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: /usr/local/google/home/kadircet/repos/llvm/build/bin/clang-17 -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all -dumpdir a- -disable-free -clear-ast-before-backend -main-file-name repro.cc -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -tune-cpu generic -debugger-tuning=gdb -fcoverage-compilation-dir=/usr/local/google/home/kadircet/repos/tmp/new_crasher -resource-dir /usr/local/google/home/kadircet/repos/llvm/build/lib/clang/18 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/x86_64-linux-gnu/c++/12 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/backward -internal-isystem /usr/local/google/home/kadircet/repos/llvm/build/lib/clang/18/include -internal-isystem /usr/local/include -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include -internal-externc-isystem /usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -fdebug-compilation-dir=/usr/local/google/home/kadircet/repos/tmp/new_crasher -ferror-limit 19 -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /tmp/repro-aa9901.o -x c++ repro.cc
1.      repro.cc:3:12: current parser token 'a'
2.      repro.cc:1:1: parsing struct/union/class body 'a'
3.      repro.cc:3:11: parsing function body 'a::b'
4.      repro.cc:3:11: in compound statement ('{}')
 #0 0x000000000746e327 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Unix/Signals.inc:602:13
 #1 0x000000000746c0ae llvm::sys::RunSignalHandlers() /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Signals.cpp:105:18
 #2 0x000000000746e9ba SignalHandler(int) /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Unix/Signals.inc:413:1
 #3 0x00007efd7465a540 (/lib/x86_64-linux-gnu/libc.so.6+0x3c540)
 #4 0x00007efd746a812c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x00007efd7465a4a2 raise ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x00007efd746444b2 abort ./stdlib/abort.c:81:7
 #7 0x00007efd746443d5 _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #8 0x00007efd746533a2 (/lib/x86_64-linux-gnu/libc.so.6+0x353a2)
 #9 0x000000000a014bf8 clang::LookupResult::checkDebugAssumptions() const /usr/local/google/home/kadircet/repos/llvm/clang/lib/Sema/SemaLookup.cpp:334:3
#10 0x0000000009b95356 getResultKind /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Sema/Lookup.h:342:5
#11 0x0000000009b95356 clang::Sema::ClassifyName(clang::Scope*, clang::CXXScopeSpec&, clang::IdentifierInfo*&, clang::SourceLocation, clang::Token const&, clang::CorrectionCandidateCallback*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Sema/SemaDecl.cpp:953:18
#12 0x00000000098a793e clang::Parser::TryAnnotateName(clang::CorrectionCandidateCallback*, clang::ImplicitTypenameContext) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:1770:53
#13 0x00000000099570dd clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&, clang::ParsedAttributes&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:214:33
#14 0x0000000009956c55 clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:117:20
#15 0x0000000009960ab1 clang::Parser::ParseCompoundStatementBody(bool) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:1205:11
#16 0x0000000009961a9d clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:2468:21
#17 0x0000000009954a3f isNot /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Lex/Token.h:99:52
#18 0x0000000009954a3f clang::Parser::ParseLexedMethodDef(clang::Parser::LexedMethod&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseCXXInlineMethods.cpp:600:14
#19 0x0000000009952e0a clang::Parser::ParseLexedMethodDefs(clang::Parser::ParsingClass&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseCXXInlineMethods.cpp:529:33
#20 0x00000000098f78e8 clang::Parser::ParseCXXMemberSpecification(clang::SourceLocation, clang::SourceLocation, clang::ParsedAttributes&, unsigned int, clang::Decl*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseDeclCXX.cpp:3674:21
#21 0x00000000098f4e9c clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseDeclCXX.cpp:0:7
#22 0x0000000009918955 empty /usr/local/google/home/kadircet/repos/llvm/llvm/include/llvm/ADT/SmallVector.h:94:46
#23 0x0000000009918955 empty /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Sema/ParsedAttr.h:819:40
#24 0x0000000009918955 clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseDecl.cpp:4327:23
#25 0x00000000098a3234 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:1123:10
#26 0x00000000098a2ed0 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:1229:12
#27 0x00000000098a1e8e clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:0:14
#28 0x000000000989fad7 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:0:12
#29 0x000000000989f27f clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:594:26
#30 0x0000000009899f6b clang::ParseAST(clang::Sema&, bool, bool) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseAST.cpp:162:5
#31 0x0000000008037930 clang::FrontendAction::Execute() /usr/local/google/home/kadircet/repos/llvm/clang/lib/Frontend/FrontendAction.cpp:1067:10
#32 0x0000000007fab25f getPtr /usr/local/google/home/kadircet/repos/llvm/llvm/include/llvm/Support/Error.h:270:42
#33 0x0000000007fab25f operator bool /usr/local/google/home/kadircet/repos/llvm/llvm/include/llvm/Support/Error.h:233:16
#34 0x0000000007fab25f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Frontend/CompilerInstance.cpp:1054:23
#35 0x000000000811efff clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:272:25
#36 0x0000000004ee5ea8 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /usr/local/google/home/kadircet/repos/llvm/clang/tools/driver/cc1_main.cpp:249:15
#37 0x0000000004ee2a41 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) /usr/local/google/home/kadircet/repos/llvm/clang/tools/driver/driver.cpp:0:12
#38 0x0000000004ee1c39 clang_main(int, char**, llvm::ToolContext const&) /usr/local/google/home/kadircet/repos/llvm/clang/tools/driver/driver.cpp:407:12
#39 0x0000000004ef2431 main /usr/local/google/home/kadircet/repos/llvm/build/tools/clang/tools/driver/clang-driver.cpp:15:3
#40 0x00007efd746456ca __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#41 0x00007efd74645785 call_init ./csu/../csu/libc-start.c:128:20
#42 0x00007efd74645785 __libc_start_main ./csu/../csu/libc-start.c:347:5
#43 0x0000000004ededa1 _start (/usr/local/google/home/kadircet/repos/llvm/build/bin/clang-17+0x4ededa1)

(note that this assertion failure is leading into crashes in NDEBUG builds).

Without this patch clang's output was:

$ ~/repos/llvm/build/bin/clang -xc++ repro.cc
repro.cc:2:10: error: unknown type name 'c'
    2 |   static c a;
      |          ^
repro.cc:2:12: error: member 'a' has the same name as its class
    2 |   static c a;
      |            ^
repro.cc:3:13: error: expected unqualified-id
    3 |   void b(){a};
      |             ^
3 errors generated

Reverting for now as this is triggering lots of crashes in our production systems.

[aaron.ballman]

In D154503#4576393, @kadircet wrote:

Hi!

After this patch clang seems to be crashing on:

class a {
  static c a;
  void b(){a};
};

with stack trace and assertion error:

$ ~/repos/llvm/build/bin/clang -xc++ repro.cc
repro.cc:2:10: error: unknown type name 'c'
    2 |   static c a;
      |          ^
repro.cc:2:12: error: member 'a' has the same name as its class
    2 |   static c a;
      |            ^
clang-17: /usr/local/google/home/kadircet/repos/llvm/clang/lib/Sema/SemaLookup.cpp:333: bool clang::LookupResult::checkDebugAssumptions() const: Assertion `ResultKind != Found || Decls.size() == 1' 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: /usr/local/google/home/kadircet/repos/llvm/build/bin/clang-17 -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all -dumpdir a- -disable-free -clear-ast-before-backend -main-file-name repro.cc -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -tune-cpu generic -debugger-tuning=gdb -fcoverage-compilation-dir=/usr/local/google/home/kadircet/repos/tmp/new_crasher -resource-dir /usr/local/google/home/kadircet/repos/llvm/build/lib/clang/18 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/x86_64-linux-gnu/c++/12 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/backward -internal-isystem /usr/local/google/home/kadircet/repos/llvm/build/lib/clang/18/include -internal-isystem /usr/local/include -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include -internal-externc-isystem /usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -fdebug-compilation-dir=/usr/local/google/home/kadircet/repos/tmp/new_crasher -ferror-limit 19 -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /tmp/repro-aa9901.o -x c++ repro.cc
1.      repro.cc:3:12: current parser token 'a'
2.      repro.cc:1:1: parsing struct/union/class body 'a'
3.      repro.cc:3:11: parsing function body 'a::b'
4.      repro.cc:3:11: in compound statement ('{}')
 #0 0x000000000746e327 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Unix/Signals.inc:602:13
 #1 0x000000000746c0ae llvm::sys::RunSignalHandlers() /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Signals.cpp:105:18
 #2 0x000000000746e9ba SignalHandler(int) /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Unix/Signals.inc:413:1
 #3 0x00007efd7465a540 (/lib/x86_64-linux-gnu/libc.so.6+0x3c540)
 #4 0x00007efd746a812c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x00007efd7465a4a2 raise ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x00007efd746444b2 abort ./stdlib/abort.c:81:7
 #7 0x00007efd746443d5 _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #8 0x00007efd746533a2 (/lib/x86_64-linux-gnu/libc.so.6+0x353a2)
 #9 0x000000000a014bf8 clang::LookupResult::checkDebugAssumptions() const /usr/local/google/home/kadircet/repos/llvm/clang/lib/Sema/SemaLookup.cpp:334:3
#10 0x0000000009b95356 getResultKind /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Sema/Lookup.h:342:5
#11 0x0000000009b95356 clang::Sema::ClassifyName(clang::Scope*, clang::CXXScopeSpec&, clang::IdentifierInfo*&, clang::SourceLocation, clang::Token const&, clang::CorrectionCandidateCallback*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Sema/SemaDecl.cpp:953:18
#12 0x00000000098a793e clang::Parser::TryAnnotateName(clang::CorrectionCandidateCallback*, clang::ImplicitTypenameContext) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:1770:53
#13 0x00000000099570dd clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&, clang::ParsedAttributes&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:214:33
#14 0x0000000009956c55 clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:117:20
#15 0x0000000009960ab1 clang::Parser::ParseCompoundStatementBody(bool) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:1205:11
#16 0x0000000009961a9d clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseStmt.cpp:2468:21
#17 0x0000000009954a3f isNot /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Lex/Token.h:99:52
#18 0x0000000009954a3f clang::Parser::ParseLexedMethodDef(clang::Parser::LexedMethod&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseCXXInlineMethods.cpp:600:14
#19 0x0000000009952e0a clang::Parser::ParseLexedMethodDefs(clang::Parser::ParsingClass&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseCXXInlineMethods.cpp:529:33
#20 0x00000000098f78e8 clang::Parser::ParseCXXMemberSpecification(clang::SourceLocation, clang::SourceLocation, clang::ParsedAttributes&, unsigned int, clang::Decl*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseDeclCXX.cpp:3674:21
#21 0x00000000098f4e9c clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseDeclCXX.cpp:0:7
#22 0x0000000009918955 empty /usr/local/google/home/kadircet/repos/llvm/llvm/include/llvm/ADT/SmallVector.h:94:46
#23 0x0000000009918955 empty /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Sema/ParsedAttr.h:819:40
#24 0x0000000009918955 clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo const&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseDecl.cpp:4327:23
#25 0x00000000098a3234 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:1123:10
#26 0x00000000098a2ed0 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:1229:12
#27 0x00000000098a1e8e clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:0:14
#28 0x000000000989fad7 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:0:12
#29 0x000000000989f27f clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/Parser.cpp:594:26
#30 0x0000000009899f6b clang::ParseAST(clang::Sema&, bool, bool) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Parse/ParseAST.cpp:162:5
#31 0x0000000008037930 clang::FrontendAction::Execute() /usr/local/google/home/kadircet/repos/llvm/clang/lib/Frontend/FrontendAction.cpp:1067:10
#32 0x0000000007fab25f getPtr /usr/local/google/home/kadircet/repos/llvm/llvm/include/llvm/Support/Error.h:270:42
#33 0x0000000007fab25f operator bool /usr/local/google/home/kadircet/repos/llvm/llvm/include/llvm/Support/Error.h:233:16
#34 0x0000000007fab25f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Frontend/CompilerInstance.cpp:1054:23
#35 0x000000000811efff clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:272:25
#36 0x0000000004ee5ea8 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /usr/local/google/home/kadircet/repos/llvm/clang/tools/driver/cc1_main.cpp:249:15
#37 0x0000000004ee2a41 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) /usr/local/google/home/kadircet/repos/llvm/clang/tools/driver/driver.cpp:0:12
#38 0x0000000004ee1c39 clang_main(int, char**, llvm::ToolContext const&) /usr/local/google/home/kadircet/repos/llvm/clang/tools/driver/driver.cpp:407:12
#39 0x0000000004ef2431 main /usr/local/google/home/kadircet/repos/llvm/build/tools/clang/tools/driver/clang-driver.cpp:15:3
#40 0x00007efd746456ca __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#41 0x00007efd74645785 call_init ./csu/../csu/libc-start.c:128:20
#42 0x00007efd74645785 __libc_start_main ./csu/../csu/libc-start.c:347:5
#43 0x0000000004ededa1 _start (/usr/local/google/home/kadircet/repos/llvm/build/bin/clang-17+0x4ededa1)

(note that this assertion failure is leading into crashes in NDEBUG builds).

Without this patch clang's output was:

$ ~/repos/llvm/build/bin/clang -xc++ repro.cc
repro.cc:2:10: error: unknown type name 'c'
    2 |   static c a;
      |          ^
repro.cc:2:12: error: member 'a' has the same name as its class
    2 |   static c a;
      |            ^
repro.cc:3:13: error: expected unqualified-id
    3 |   void b(){a};
      |             ^
3 errors generated

Reverting for now as this is triggering lots of crashes in our production systems.

I'm not opposed to a revert if this is causing problems for your downstream, but be sure to also remove the changes from the 17.x branch if you go this route rather than fixing forward.

[kadircet]

In D154503#4576442, @aaron.ballman wrote:

I'm not opposed to a revert if this is causing problems for your downstream, but be sure to also remove the changes from the 17.x branch if you go this route rather than fixing forward.

Thanks! I didn't notice it made into the release branch. I'd wait for author's comment on this one, as I guess we can cherry-pick just the fix-forward if it turns out to be small and safe. Otherwise I am more than happy to request the cherry-pick for the revert.

[aaron.ballman]

In D154503#4576475, @kadircet wrote:

In D154503#4576442, @aaron.ballman wrote:

I'm not opposed to a revert if this is causing problems for your downstream, but be sure to also remove the changes from the 17.x branch if you go this route rather than fixing forward.

Thanks! I didn't notice it made into the release branch. I'd wait for author's comment on this one, as I guess we can cherry-pick just the fix-forward if it turns out to be small and safe. Otherwise I am more than happy to request the cherry-pick for the revert.

SGTM!

[kadircet]

In D154503#4576481, @aaron.ballman wrote:

In D154503#4576475, @kadircet wrote:

In D154503#4576442, @aaron.ballman wrote:

I'm not opposed to a revert if this is causing problems for your downstream, but be sure to also remove the changes from the 17.x branch if you go this route rather than fixing forward.

Thanks! I didn't notice it made into the release branch. I'd wait for author's comment on this one, as I guess we can cherry-pick just the fix-forward if it turns out to be small and safe. Otherwise I am more than happy to request the cherry-pick for the revert.

SGTM!

Requested cherry-pick in https://github.com/llvm/llvm-project/issues/64670

[john.brawn]

I've committed a test for the behaviour when we have invalid decls in https://reviews.llvm.org/rG6244e3840694.

[john.brawn]

I've committed an updated version of this that checks for invalid decls when deciding if a decl has been hidden. This stops the assertion failure happening.

clang/lib/Sema/SemaLookup.cpp

if (N == 1) {
     else if (isa<UnresolvedUsingValueDecl>(D))
       ResultKind = FoundUnresolvedValue;
     return;
   }
 
   // Don't do any extra resolution if we've already resolved as ambiguous.
   if (ResultKind == Ambiguous) return;
 
   llvm::SmallDenseMap<const NamedDecl *, unsigned, 16> Unique;
   llvm::SmallDenseMap<QualType, unsigned, 16> UniqueTypes;
 
   bool Ambiguous = false;
   bool HasTag = false, HasFunction = false;
   bool HasFunctionTemplate = false, HasUnresolved = false;
   const NamedDecl *HasNonFunction = nullptr;
 
   llvm::SmallVector<const NamedDecl *, 4> EquivalentNonFunctions;
+  llvm::BitVector RemovedDecls(N);
 
-  unsigned UniqueTagIndex = 0;
-
-  unsigned I = 0;
-  while (I < N) {
+  for (unsigned I = 0; I < N; I++) {
     const NamedDecl *D = Decls[I]->getUnderlyingDecl();
     D = cast<NamedDecl>(D->getCanonicalDecl());
 
     // Ignore an invalid declaration unless it's the only one left.
     // Also ignore HLSLBufferDecl which not have name conflict with other Decls.
-    if ((D->isInvalidDecl() || isa<HLSLBufferDecl>(D)) && !(I == 0 && N == 1)) {
-      Decls[I] = Decls[--N];
+    if ((D->isInvalidDecl() || isa<HLSLBufferDecl>(D)) &&
+        N - RemovedDecls.count() > 1) {
+      RemovedDecls.set(I);
+      continue;
+    }
+
+    // C++ [basic.scope.hiding]p2:
+    //   A class name or enumeration name can be hidden by the name of
+    //   an object, function, or enumerator declared in the same
+    //   scope. If a class or enumeration name and an object, function,
+    //   or enumerator are declared in the same scope (in any order)
+    //   with the same name, the class or enumeration name is hidden
+    //   wherever the object, function, or enumerator name is visible.
+    if (HideTags && isa<TagDecl>(D)) {
+      bool Hidden = false;
+      for (auto *OtherDecl : Decls) {
+        if (canHideTag(OtherDecl) &&
+            getContextForScopeMatching(OtherDecl)->Equals(
+                getContextForScopeMatching(Decls[I]))) {
+          RemovedDecls.set(I);
+          Hidden = true;
+          break;
+        }
+      }
+      if (Hidden)
         continue;
     }
 
     std::optional<unsigned> ExistingI;
 
     // Redeclarations of types via typedef can occur both within a scope
     // and, through using declarations and directives, across scopes. There is
     // no ambiguity if they all refer to the same type, so unique based on the
     // canonical type.
if (!ExistingI) {
       }
     }
 
     if (ExistingI) {
       // This is not a unique lookup result. Pick one of the results and
       // discard the other.
       if (isPreferredLookupResult(getSema(), getLookupKind(), Decls[I],
                                   Decls[*ExistingI]))
-        Decls[*ExistingI] = Decls[I];
-      Decls[I] = Decls[--N];
+        RemovedDecls.set(*ExistingI);
+      else
+        RemovedDecls.set(I);
       continue;
     }
 
     // Otherwise, do some decl type analysis and then continue.
 
     if (isa<UnresolvedUsingValueDecl>(D)) {
       HasUnresolved = true;
     } else if (isa<TagDecl>(D)) {
       if (HasTag)
         Ambiguous = true;
-      UniqueTagIndex = I;
       HasTag = true;
     } else if (isa<FunctionTemplateDecl>(D)) {
       HasFunction = true;
       HasFunctionTemplate = true;
     } else if (isa<FunctionDecl>(D)) {
       HasFunction = true;
     } else {
       if (HasNonFunction) {
         // If we're about to create an ambiguity between two declarations that
         // are equivalent, but one is an internal linkage declaration from one
         // module and the other is an internal linkage declaration from another
         // module, just skip it.
         if (getSema().isEquivalentInternalLinkageDeclaration(HasNonFunction,
                                                              D)) {
           EquivalentNonFunctions.push_back(D);
-          Decls[I] = Decls[--N];
+          RemovedDecls.set(I);
           continue;
         }
 
         Ambiguous = true;
       }
       HasNonFunction = D;
     }
-    I++;
-  }
-
-  // C++ [basic.scope.hiding]p2:
-  //   A class name or enumeration name can be hidden by the name of
-  //   an object, function, or enumerator declared in the same
-  //   scope. If a class or enumeration name and an object, function,
-  //   or enumerator are declared in the same scope (in any order)
-  //   with the same name, the class or enumeration name is hidden
-  //   wherever the object, function, or enumerator name is visible.
-  // But it's still an error if there are distinct tag types found,
-  // even if they're not visible. (ref?)
-  if (N > 1 && HideTags && HasTag && !Ambiguous &&
-      (HasFunction || HasNonFunction || HasUnresolved)) {
-    const NamedDecl *OtherDecl = Decls[UniqueTagIndex ? 0 : N - 1];
-    if (isa<TagDecl>(Decls[UniqueTagIndex]->getUnderlyingDecl()) &&
-        getContextForScopeMatching(Decls[UniqueTagIndex])->Equals(
-            getContextForScopeMatching(OtherDecl)) &&
-        canHideTag(OtherDecl))
-      Decls[UniqueTagIndex] = Decls[--N];
-    else
-      Ambiguous = true;
   }
 
   // FIXME: This diagnostic should really be delayed until we're done with
   // the lookup result, in case the ambiguity is resolved by the caller.
   if (!EquivalentNonFunctions.empty() && !Ambiguous)
     getSema().diagnoseEquivalentInternalLinkageDeclarations(
         getNameLoc(), HasNonFunction, EquivalentNonFunctions);
 
+  // Remove decls by replacing them with decls from the end (which
+  // means that we need to iterate from the end) and then truncating
+  // to the new size.
+  for (int I = RemovedDecls.find_last(); I >= 0; I = RemovedDecls.find_prev(I))
+    Decls[I] = Decls[--N];
   Decls.truncate(N);
 
-  if (HasNonFunction && (HasFunction || HasUnresolved))
+  if ((HasNonFunction && (HasFunction || HasUnresolved)) ||
+      (HideTags && HasTag && (HasFunction || HasNonFunction || HasUnresolved)))
     Ambiguous = true;
 
   if (Ambiguous)
     setAmbiguous(LookupResult::AmbiguousReference);
   else if (HasUnresolved)
     ResultKind = LookupResult::FoundUnresolvedValue;
   else if (N > 1 || HasFunctionTemplate)
     ResultKind = LookupResult::FoundOverloaded;

clang/test/SemaCXX/using-hiding.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+namespace A {
+  class X { }; // expected-note{{candidate found by name lookup is 'A::X'}}
+               // expected-note@-1{{candidate found by name lookup is 'A::X'}}
+}
+namespace B {
+  void X(int); // expected-note{{candidate found by name lookup is 'B::X'}}
+               // expected-note@-1{{candidate found by name lookup is 'B::X'}}
+}
+
+// Using directive doesn't cause A::X to be hidden, so X is ambiguous.
+namespace Test1a {
+  using namespace A;
+  using namespace B;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test1b {
+  using namespace B;
+  using namespace A;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+// The behaviour here should be the same as using namespaces A and B directly
+namespace Test2a {
+  namespace C {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test2a::C::X'}}
+  }
+  namespace D {
+    using B::X; // expected-note{{candidate found by name lookup is 'Test2a::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test2b {
+  namespace C {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test2b::C::X'}}
+  }
+  namespace D {
+    using B::X; // expected-note{{candidate found by name lookup is 'Test2b::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+// Defining a function X inside C should hide using A::X in C but not D, so the result is ambiguous.
+namespace Test3a {
+  namespace C {
+    using A::X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3a::C::X'}}
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3a::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test3b {
+  namespace C {
+    using A::X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3b::C::X'}}
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3b::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test3c {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3c::C::X'}}
+    using A::X;
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3c::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test3d {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test3d::C::X'}}
+    using A::X;
+  }
+  namespace D {
+    using A::X; // expected-note{{candidate found by name lookup is 'Test3d::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+// A::X hidden in both C and D by overloaded function, so the result is not ambiguous.
+namespace Test4a {
+  namespace C {
+    using A::X;
+    void X(int);
+  }
+  namespace D {
+    using A::X;
+    void X(int, int);
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test4b {
+  namespace C {
+    using A::X;
+    void X(int);
+  }
+  namespace D {
+    using A::X;
+    void X(int, int);
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test4c {
+  namespace C {
+    void X(int);
+    using A::X;
+  }
+  namespace D {
+    void X(int, int);
+    using A::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test4d {
+  namespace C {
+    void X(int);
+    using A::X;
+  }
+  namespace D {
+    void X(int, int);
+    using A::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+// B::X hides class X in C, so the the result is not ambiguous
+namespace Test5a {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test5b {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test5c {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test5d {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    using B::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+// B::X hides class X declared in both C and D, so the result is not ambiguous.
+namespace Test6a {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    class X { };
+    using B::X;
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test6b {
+  namespace C {
+    class X { };
+    using B::X;
+  }
+  namespace D {
+    class X { };
+    using B::X;
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test6c {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+    class X { };
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1);
+  }
+}
+
+namespace Test6d {
+  namespace C {
+    using B::X;
+    class X { };
+  }
+  namespace D {
+    using B::X;
+    class X { };
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1);
+  }
+}
+
+// function X inside C should hide class X in C but not D.
+namespace Test7a {
+  namespace C {
+    class X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7a::C::X'}}
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7a::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test7b {
+  namespace C {
+    class X;
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7b::C::X'}}
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7b::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test7c {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7c::C::X'}}
+    class X;
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7c::D::X'}}
+  }
+  using namespace C;
+  using namespace D;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}
+
+namespace Test7d {
+  namespace C {
+    void X(int); // expected-note{{candidate found by name lookup is 'Test7d::C::X'}}
+    class X;
+  }
+  namespace D {
+    class X; // expected-note{{candidate found by name lookup is 'Test7d::D::X'}}
+  }
+  using namespace D;
+  using namespace C;
+  void f() {
+    X(1); // expected-error{{reference to 'X' is ambiguous}}
+  }
+}