add boundary check for ASTUnresolvedSet::erase

[zhouyizhou]

Summary

When compile following code with clang (Debug build), Assertion will be triggered.

struct A
{
        struct Nested {};
        operator Nested*() {return 0;};
};

struct B : A
{
        using A::operator typename A::Nested*;
        operator typename A::Nested *() {
                struct A * thi = this;
                return *thi;
        };
};

The assertion fail is caused by: void erase(unsigned I) { Decls[I] = Decls.pop_back_val(); } when size of Decls is 1 before erase.

Thanks for review my patch
Zhouyi Zhou
[e-mail]

[zhouyizhou]

We should also consider the situation of erasing the size - 1th element

[zhouyizhou]

Following is the back trace when the assertion fires before my fix:
#6 0x00007ffff7960e96 in GI_assert_fail (assertion=0x5555656ed285 "Begin + idx < End",

file=0x5555656ed240 "/tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTVector.h", line=112, 
function=0x5555656ed1b0 "T& clang::ASTVector<T>::operator[](unsigned int) [with T = clang::DeclAccessPair; clang::ASTVector<T>::reference = clang::DeclAccessPair&]") at ./assert/assert.c:101

#7 0x000055555f448c05 in clang::ASTVector<clang::DeclAccessPair>::operator[] (this=0x5555691a19b8, idx=0)

at /tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTVector.h:112

#8 0x000055555f446e26 in clang::ASTUnresolvedSet::erase (this=0x5555691a19b8, I=0)

at /tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTUnresolvedSet.h:72

#9 0x000055555f43e7a3 in clang::CXXRecordDecl::removeConversion (this=0x5555691a18d8, ConvDecl=0x5555691a1c48)

at /tmp/llvm-test/llvm-project.save/clang/lib/AST/DeclCXX.cpp:1797

#10 0x000055555e121349 in clang::Sema::HideUsingShadowDecl (this=0x55556918f990, S=0x5555691acce0, Shadow=0x5555691a1c48)

at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDeclCXX.cpp:12158

#11 0x000055555e80bc89 in clang::Sema::CheckOverload (this=0x55556918f990, S=0x5555691acce0, New=0x5555691a1da0, Old=...,

Match=@0x7fffffff72b8: 0x0, NewIsUsingDecl=false) at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaOverload.cpp:1063

#12 0x000055555dfdb925 in clang::Sema::CheckFunctionDeclaration (this=0x55556918f990, S=0x5555691acce0, NewFD=0x5555691a1da0,

Previous=..., IsMemberSpecialization=false, DeclIsDefn=true)
at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDecl.cpp:11356

#13 0x000055555dfd64f3 in clang::Sema::ActOnFunctionDeclarator (this=0x55556918f990, S=0x5555691acce0, D=...,

DC=0x5555691a1918, TInfo=0x5555691a1d68, Previous=..., TemplateParamListsRef=..., AddToScope=@0x7fffffff7b30: true)
at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDecl.cpp:10205

#14 0x000055555dfc57cb in clang::Sema::HandleDeclarator (this=0x55556918f990, S=0x5555691acce0, D=..., TemplateParamLists=...)

at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDecl.cpp:6348

--Type <RET> for more, q to quit, c to continue without paging--
#15 0x000055555e0fa7bc in clang::Sema::ActOnCXXMemberDeclarator (this=0x55556918f990, S=0x5555691acce0, AS=clang::AS_public,

D=..., TemplateParameterLists=..., BW=0x0, VS=..., InitStyle=clang::ICIS_NoInit)
at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDeclCXX.cpp:3477

#16 0x000055555dc5b40a in clang::Parser::ParseCXXInlineMethodDef (this=0x55556919b200, AS=clang::AS_public, AccessAttrs=...,

D=..., TemplateInfo=..., VS=..., PureSpecLoc=...)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseCXXInlineMethods.cpp:42

#17 0x000055555dc9b22b in clang::Parser::ParseCXXClassMemberDeclaration (this=0x55556919b200, AS=clang::AS_public,

AccessAttrs=..., TemplateInfo=..., TemplateDiags=0x0)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:2912

#18 0x000055555dc9d149 in clang::Parser::ParseCXXClassMemberDeclarationWithPragmas (this=0x55556919b200,

AS=@0x7fffffff94f0: clang::AS_public, AccessAttrs=..., TagType=clang::TST_struct, TagDecl=0x5555691a18d8)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:3343

#19 0x000055555dc9df12 in clang::Parser::ParseCXXMemberSpecification (this=0x55556919b200, RecordLoc=..., AttrFixitLoc=...,

Attrs=..., TagType=25, TagDecl=0x5555691a18d8) at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:3547

#20 0x000055555dc97ce3 in clang::Parser::ParseClassSpecifier (this=0x55556919b200, TagTokKind=clang::tok::kw_struct,

StartLoc=..., DS=..., TemplateInfo=..., AS=clang::AS_none, EnteringContext=true, 
DSC=clang::Parser::DeclSpecContext::DSC_top_level, Attributes=...)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:2063

#21 0x000055555dc7504a in clang::Parser::ParseDeclarationSpecifiers (this=0x55556919b200, DS=..., TemplateInfo=...,

AS=clang::AS_none, DSContext=clang::Parser::DeclSpecContext::DSC_top_level, LateAttrs=0x0)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDecl.cpp:4144

#22 0x000055555dc4a70e in clang::Parser::ParseDeclOrFunctionDefInternal (this=0x55556919b200, Attrs=..., DS=...,

AS=clang::AS_none) at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:1087

#23 0x000055555dc4adb1 in clang::Parser::ParseDeclarationOrFunctionDefinition (this=0x55556919b200, Attrs=..., DS=0x0,

AS=clang::AS_none) at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:1193

#24 0x000055555dc4a260 in clang::Parser::ParseExternalDeclaration (this=0x55556919b200, Attrs=..., DS=0x0)

at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:1019

#25 0x000055555dc48fac in clang::Parser::ParseTopLevelDecl (this=0x55556919b200, Result=...,

ImportState=@0x7fffffffa97c: clang::Sema::ModuleImportState::NotACXX20Module)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:737

#26 0x000055555dc43f29 in clang::ParseAST (S=..., PrintStats=false, SkipFunctionBodies=false)
--Type <RET> for more, q to quit, c to continue without paging--

at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseAST.cpp:162

#27 0x000055555b4c6aaf in clang::ASTFrontendAction::ExecuteAction (this=0x55556911d8e0)

at /tmp/llvm-test/llvm-project.save/clang/lib/Frontend/FrontendAction.cpp:1152

#28 0x000055555bebe736 in clang::CodeGenAction::ExecuteAction (this=0x55556911d8e0)

at /tmp/llvm-test/llvm-project.save/clang/lib/CodeGen/CodeGenAction.cpp:1144

#29 0x000055555b4c635e in clang::FrontendAction::Execute (this=0x55556911d8e0)

at /tmp/llvm-test/llvm-project.save/clang/lib/Frontend/FrontendAction.cpp:1045

#30 0x000055555b3f45cd in clang::CompilerInstance::ExecuteAction (this=0x555569118ba0, Act=...)

at /tmp/llvm-test/llvm-project.save/clang/lib/Frontend/CompilerInstance.cpp:1039

#31 0x000055555b667bce in clang::ExecuteCompilerInvocation (Clang=0x555569118ba0)

at /tmp/llvm-test/llvm-project.save/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266

#32 0x0000555556c05a6d in cc1_main (Argv=..., Argv0=0x7fffffffdbc0 "/tmp/llvm-test/llvm-project.save/build/bin/clang-16",

MainAddr=0x555556bf616a <GetExecutablePath[abi:cxx11](char const*, bool)>)
at /tmp/llvm-test/llvm-project.save/clang/tools/driver/cc1_main.cpp:250

#33 0x0000555556bf79ef in ExecuteCC1Tool (ArgV=...) at /tmp/llvm-test/llvm-project.save/clang/tools/driver/driver.cpp:322
#34 0x0000555556bf81a0 in clang_main (Argc=64, Argv=0x7fffffffd628)

at /tmp/llvm-test/llvm-project.save/clang/tools/driver/driver.cpp:397

#35 0x0000555556c252a1 in main (argc=64, argv=0x7fffffffd628)

at /tmp/llvm-test/llvm-project.save/build/tools/clang/tools/driver/clang-driver.cpp:11

[rjmccall]

Good catch! Easy to see how this escaped notice for a decade, but still, it'll be good to fix.

LGTM with a minor improvement.

clang/include/clang/AST/ASTUnresolvedSet.h

74

This can just be pop_back().

[zhouyizhou]

In D137263#3903845, @rjmccall wrote:

Good catch! Easy to see how this escaped notice for a decade, but still, it'll be good to fix.

LGTM with a minor improvement.

Thank John for your encouragement and guidance!
I have no write access to LLVM project, could you please commit it for me when you are free?

Cheers
Zhouyi Zhou
[e-mail]

[MaskRay]

The summary and comments use Markdown. You comment is currently formatted strangely because you did not use fenced code blocks :)
(The commit message does not necessarily use Markdown. Since the summary comes from the commit message, many just use Markdown unconditionally and that is fine.)

[zhouyizhou]

In D137263#3904184, @MaskRay wrote:

The summary and comments use Markdown. You comment is currently formatted strangely because you did not use fenced code blocks :)
(The commit message does not necessarily use Markdown. Since the summary comes from the commit message, many just use Markdown unconditionally and that is fine.)

Thank Ray for your precious advice!

Sorry for the trouble that I have made, I will learn to use fenced code blocks next time ( I am a beginner who eager to learn ;-))!

Sincerely yours
Zhouyi

[zhouyizhou]

Thank Ray for editing the summary for me ;-) I learned to use fenced code blocks now from your example.
Thank you both!

[MaskRay]

We need a regression test. See clang/test/SemaCXX/using-decl* for some other using declaration tests. Study some tests, find a file which is appropriate, and add a reduced case there.
Use ninja check-clang-semacxx to run clang/test/SemaCXX tests. Use $build/bin/llvm-lit -v clang/test/SemaCXX/using-decl-1.cpp to run one test.

I find that if I comment out cast<CXXRecordDecl>(Shadow->getDeclContext())->removeConversion(Shadow); in Sema::HideUsingShadowDecl, no test fails... So we have a missing coverage issue.

clang-14 build on Ubuntu 22.04 don't trigger above assertion because clang-14 using g++ -std=c++14 by default:

The instruction and the diassembly is irrelevant to this fix and can be removed.

[zhouyizhou]

In D137263#3909722, @MaskRay wrote:

Thank Ray for your guidance!

We need a regression test. See clang/test/SemaCXX/using-decl* for some other using declaration tests. Study some tests, find a file which is appropriate, and add a reduced case there.
Use ninja check-clang-semacxx to run clang/test/SemaCXX tests. Use $build/bin/llvm-lit -v clang/test/SemaCXX/using-decl-1.cpp to run one test.

I find that if I comment out cast<CXXRecordDecl>(Shadow->getDeclContext())->removeConversion(Shadow); in Sema::HideUsingShadowDecl, no test fails... So we have a missing coverage issue.

OK, I will try to accomplish all above. There are a lot to learn for me, so please wait for my completion which may take relatively a long time (maybe about 2 weeks).

clang-14 build on Ubuntu 22.04 don't trigger above assertion because clang-14 using g++ -std=c++14 by default:

The instruction and the diassembly is irrelevant to this fix and can be removed.

Done

Thanks again

Sincerely
Zhouyi

[zhouyizhou]

In D137263#3909722, @MaskRay wrote:

I find that if I comment out cast<CXXRecordDecl>(Shadow->getDeclContext())->removeConversion(Shadow); in Sema::HideUsingShadowDecl, no test fails... So we have a missing coverage issue.

Hi
I found a counterexample for above case, excited! ! and I could not hesitate to report you that good news ;-)

struct A
{
        struct Nested {};
        operator Nested*() {return 0;};
};

struct B : A
{
        using A::operator typename A::Nested*;
        operator typename A::Nested *() {
                struct A * thi = this;
                return *thi;
        };
};

int
main()
{

        struct B b;
        auto s = *b;

}

After comment out cast<CXXRecordDecl>(Shadow->getDeclContext())->removeConversion(Shadow);
clang++ report following:

using.C:21:11: error: use of overloaded operator '*' is ambiguous (operand type 'struct B')
        auto s = *b;
                 ^~
using.C:21:11: note: because of ambiguity in conversion of 'struct B' to 'A::Nested *'
using.C:4:9: note: candidate function
        operator Nested*() {return 0;};
        ^
using.C:10:9: note: candidate function
        operator typename A::Nested *() {
        ^
using.C:21:11: note: built-in candidate operator*(struct A::Nested *)
        auto s = *b;
                 ^
using.C:21:11: note: built-in candidate operator*(const struct A::Nested *)
1 error generated.

So I think we could add above to the test case.
What's your opinion?

I learned a lot under your guidance!

Thank you both
Sincerely
Zhouyi

[zhouyizhou]

add a test case
thank you ;-)

[MaskRay]

Thanks!

clang/include/clang/AST/ASTUnresolvedSet.h

73

if (I == Decls.size() - 1)

clang/test/SemaCXX/using-decl-templates.cpp

107

The llvm style prefers { at the end of the previous line. Tests usually don't need to follow the convention but the previous code in this test file follows the convention.

123

int foo() {

[zhouyizhou]

Thank Ray for your guidance!
I achieved a lot in the process ;-)

Thanks
Zhouyi

clang/include/clang/AST/ASTUnresolvedSet.h

for (DeclsTy::iterator I = Decls.begin(), E = Decls.end(); I != E; ++I) {
       if (I->getDecl() == Old) {
         I->set(New, AS);
         return true;
       }
     }
     return false;
   }
 
-  void erase(unsigned I) { Decls[I] = Decls.pop_back_val(); }
+  void erase(unsigned I) {
+    if (I == Decls.size() - 1)
+      Decls.pop_back();
+    else
+      Decls[I] = Decls.pop_back_val();
+  }
 
   void clear() { Decls.clear(); }
 
   bool empty() const { return Decls.empty(); }
   unsigned size() const { return Decls.size(); }
 
   void reserve(ASTContext &C, unsigned N) {
     Decls.reserve(C, N);

clang/test/SemaCXX/using-decl-templates.cpp

 template <bool Value> struct Base {
   static_assert(Value, ""); // expected-error {{static assertion failed}}
 };
 struct Derived : Base<false> { // expected-note {{requested here}}
   using Base<false>::Base; // OK. Don't diagnose that 'Base' isn't a base class of Derived.
 };
 } // namespace DontDiagnoseInvalidTest
 
+namespace shadow_nested_operator {
+template <typename T>
+struct A {
+  struct Nested {};
+  operator Nested*() {return 0;};
+};
+
+template <typename T>
+struct B : A<T> {
+  using A<T>::operator typename A<T>::Nested*;
+  operator typename A<T>::Nested *() {
+    struct A<T> * thi = this;
+    return *thi;
+ };
+};
+
+int foo () {
+  struct B<int> b;
+  auto s = *b;
+}
+} // namespace shadow_nested_operator
+
 namespace func_templ {
 namespace sss {
 double foo(int, double);
 template <class T>
 T foo(T);
 } // namespace sss
 
 namespace oad {