Fix for incorrect source position of dependent c'tor initializer (bug:26195)

[Serge_Preis]

Summary

This patch fixes incorrect source positions of dependent c'tor initializers like in the following code:

template<typename MyBase>
struct Derived: MyBase::InnerIterator
{

Derived() : MyBase::InnerIterator() {}  /// This line is problematic: all positions point to InnerIterator and nothing points to MyBase

};

Bug is described at https://bugs.llvm.org/show_bug.cgi?id=26195 in comments. The issue affects TypeLoc for 'MyBase', NestedNameSpecifierLoc for 'MyBase' and CXXCtorInitializer for MyBase::InnerIterator. Reproducing matcher and source codes for bad (dependent name) and good (indepent name) behaviors are attached. Reports of bad, good and bad with patch are given in comments for comparison.

The patch basically replicates code found later in routine for ElaboratedTypeLoc and adapted to DependentTypeLoc returned from CheckTypenameType().

[malcolm.parsons]

Is it possible to add a test for this change?
There are some source range checks in test/Misc/ast-dump-decl.cpp

[Serge_Preis]

In D32439#736503, @malcolm.parsons wrote:

Is it possible to add a test for this change?
There are some source range checks in test/Misc/ast-dump-decl.cpp

Yes, I think testing is quite possible. I will take a look into test/Misc/ast-dump-decl.cpp and try to come up with something meaningful.

The code exposed this issue for me looked like this:

template<typename EntityT>
bool checkNameAtPos(const EntityT entity) {
     std::string name = GetName(entity);  /// EntityT-dependent way to discover name 
     SourceLoc start = GetStart(entity);    /// entity.getBeginLoc() by default with rare exceptions

    const char* buf = SourceMgr.getCharacterData(start, &err);
    assert(!err && "getCharacterData() failed");

    if (buf[0] != name[0]) {
        llvm::errs() << "Anchor text doesn't match the symbol:\n"
                        << "  Name = " << nameStr << "\n"
                        << "  @Anchor = " << bufStr << "\n"
        start.print(llvm::errs(), SourceMgr);
        llvm::errs() << '\n';
        return false;
    }
    return true;
}

I wanted to be sure that there will be a name of an enity in the entity's location (at least prefix of entity's name). So tesing for something like this may be added for entities being TypeLoc, NestedNameSpecifier and CXXCtorInitializer.

[Serge_Preis]

In D32439#736503, @malcolm.parsons wrote:

Is it possible to add a test for this change?
There are some source range checks in test/Misc/ast-dump-decl.cpp

I looked into test/Misc/ast-dump-decl.cpp and other tests around, but they all use -ast-dump which doesn't dump source locations of affected entities (at least for problematic codes I've got). The only tool that exposed the difference is c-index-test -test-load-source all bug.cpp
bug.cpp:

template<typename MyBase>
struct Derived:  MyBase::InnerIterator
{
    Derived() : MyBase::InnerIterator() {}
};

Bad (trunk) has:

// CHECK: bug.cpp:4:25: TypeRef=MyBase:1:19 Extent=[4:25 - 4:38]

Good (patched) has:

// CHECK: bug.cpp:4:17: TypeRef=MyBase:1:19 Extent=[4:17 - 4:23]

I am new to clang development, but think that this difference can be exploited to craft proper test.

Something like:

// RUN: c-index-test -test-load-source all %s | FileCheck %s
template<typename MyBase>
struct Derived:  MyBase::InnerIterator
{
    Derived() : MyBase::InnerIterator() {}
// CHECK:  TypeRef=MyBase:1:19 Extent=[4:17 - 4:23]
};

What do you think?
I may add some other cases similar to this to the test including other variants of dependent and independent names.

[malcolm.parsons]

In D32439#736631, @Serge_Preis wrote:

What do you think?

Seems reasonable.

[Serge_Preis]

• Minor improvement to the fix: ensure that TypeLoc is of expected kind.
• Added test for various situations causing problems with source poisition in current implementation

[Serge_Preis]

Hello Richard,

This is just friendly ping. Would you please review my fix for source position issue in CXXCtorInitializer and related structures or nominate someone as appropriate reviewer for this code.

Thank you,
Serge.

[Serge_Preis]

Hello,

would you please review my small fix for incorrect source positions for certain entities. The changes were submitted more than a month ago and I understand you all are busy people, but I hope this review won't take long.

Thank you,
Serge.

[arphaman]

Thanks for the fix! The implementation looks correct. A couple of misc comments:

• Please move the test to test/Index (Since it uses c-index-test).
• In the future please upload patches with full context (git diff -U9999).

lib/Sema/SemaDeclCXX.cpp

3771

This line violates the 80 columns. Please use clang-format!

[Serge_Preis]

Fixed review comments:

• Style fixed for long line
• Test moved to test/Index
• Patch context spans entire files

[Serge_Preis]

Alex,

If changes are OK would you please commit them on my behalf (I don't have commit permissions perosonally). If this is impossible/inconvenient for any reason please let me know, I will ask Dmitry Polukhin (collegue of mine) to commit (he's done this before).

Thank you for review,
Serge.

[arphaman]

This LGTM. I will commit it for you.

[Serge_Preis]

Thank you!

[arphaman]

Serge, the test seems to be failing on linux (http://bb.pgr.jp/builders/test-clang-msc-x64-on-i686-linux-RA). Can you take a look? I will have to revert it if we can't fix it soon.

[arphaman]

Link to failure: http://bb.pgr.jp/builders/test-clang-msc-x64-on-i686-linux-RA/builds/3777/steps/test_clang/logs/Clang%20%3A%3A%20Index__ctor-init-source-loc.cpp

[arphaman]

I reverted it in r306111 for now.

[Serge_Preis]

Is there any meaning in 'test-clang-msc-x64-on-i686-linux-RA' (I just don't knwo what this configuration mean) - I tested on Linux that I have at my disposal and tesing passes.

[chapuni]

In D32439#790248, @Serge_Preis wrote:

Is there any meaning in 'test-clang-msc-x64-on-i686-linux-RA' (I just don't knwo what this configuration mean) - I tested on Linux that I have at my disposal and tesing passes.

It's cross-compiling.
LLVM_BUILD_32_BITS=ON (-m32)
LLVM_DEFAULT_TARGET_TRIPLE=x86_64-pc-win32

See also; http://bb.pgr.jp/builders/test-clang-msc-x64-on-i686-linux-RA/builds/3858

[Serge_Preis]

In D32439#790252, @chapuni wrote:

In D32439#790248, @Serge_Preis wrote:

Is there any meaning in 'test-clang-msc-x64-on-i686-linux-RA' (I just don't knwo what this configuration mean) - I tested on Linux that I have at my disposal and tesing passes.

It's cross-compiling.
LLVM_BUILD_32_BITS=ON (-m32)
LLVM_DEFAULT_TARGET_TRIPLE=x86_64-pc-win32

See also; http://bb.pgr.jp/builders/test-clang-msc-x64-on-i686-linux-RA/builds/3858

Thank you for clarifications, would you explain how may I reproduce failure locally or remotely. I'd like to see c-index-test output before clang-check to understand what's going on.

[chapuni]

In D32439#790302, @Serge_Preis wrote:

Thank you for clarifications, would you explain how may I reproduce failure locally or remotely. I'd like to see c-index-test output before clang-check to understand what's going on.

Just change LLVM_DEFAULT_TARGET_TRIPLE, build and run check-clang. You may use ccmake for it.
Or, modify RUN line locally like

RUN: c-index-test (snip) %s -Xclang -triple -Xclang x86_64-pc-win32

Don't forget restore LLVM_DEFAULT_TARGET_TRIPLE to the default value (usually it's same as HOST_TRIPLE) later.

[Serge_Preis]

In D32439#790323, @chapuni wrote:

Just change LLVM_DEFAULT_TARGET_TRIPLE, build and run check-clang. You may use ccmake for it.
Or, modify RUN line locally like

RUN: c-index-test (snip) %s -Xclang -triple -Xclang x86_64-pc-win32

Don't forget restore LLVM_DEFAULT_TARGET_TRIPLE to the default value (usually it's same as HOST_TRIPLE) later.

Thank you for your help. Unfortunately I still cannot reproduce the issue: with local correction test passes and all attempts to configure cmake lead to missing bits/c++config.h file.

[Serge_Preis]

Thank you all for your help! The problem was that in Windows-targeted builds -fdelayed-template-parsing is set by default affecting template symbols appearance in index. Throwing -fno-delayed-template-parsing to c-index-test fixed the situation for failed configuration. The regular Linux build works as before. So attaching fixed patch for 2nd attempt.

[Serge_Preis]

-fno-delayed-template-parsing atted to c-index-test arguments for consistent testing results among all targets

[arphaman]

Thanks! I'll recommit today.

[arphaman]

Recommitted in r306392.

[chapuni]

@Serge_Preis @arphaman Thanks!

lib/Sema/SemaDeclCXX.cpp

if (!TyD) {
       // We don't want access-control diagnostics here.
       R.suppressDiagnostics();
 
       if (SS.isSet() && isDependentScopeSpecifier(SS)) {
         bool NotUnknownSpecialization = false;
         DeclContext *DC = computeDeclContext(SS, false);
         if (CXXRecordDecl *Record = dyn_cast_or_null<CXXRecordDecl>(DC)) 
           NotUnknownSpecialization = !Record->hasAnyDependentBases();
 
         if (!NotUnknownSpecialization) {
           // When the scope specifier can refer to a member of an unknown
           // specialization, we take it as a type name.
           BaseType = CheckTypenameType(ETK_None, SourceLocation(),
                                        SS.getWithLocInContext(Context),
                                        *MemberOrBase, IdLoc);
           if (BaseType.isNull())
             return true;
 
+          TInfo = Context.CreateTypeSourceInfo(BaseType);
+          DependentNameTypeLoc TL =
+              TInfo->getTypeLoc().castAs<DependentNameTypeLoc>();
+          if (!TL.isNull()) {
+            TL.setNameLoc(IdLoc);
+            TL.setElaboratedKeywordLoc(SourceLocation());
+            TL.setQualifierLoc(SS.getWithLocInContext(Context));
+          }
+
           R.clear();
           R.setLookupName(MemberOrBase);
         }
       }
 
       // If no results were found, try to correct typos.
       TypoCorrection Corr;
       if (R.empty() && BaseType.isNull() &&

test/Index/ctor-init-source-loc.cpp

+// RUN: c-index-test -test-load-source all %s -fno-delayed-template-parsing | FileCheck %s
+template<typename MyBase>
+struct Derived:  MyBase::InnerIterator
+{
+    Derived() : MyBase::InnerIterator() {}
+// CHECK:  TypeRef=MyBase:2:19 Extent=[5:17 - 5:23]
+};
+
+template<typename MyBase>
+struct Derived2:  MyBase::Deeper::InnerIterator
+{
+    Derived2() : MyBase::Deeper::InnerIterator() {}
+// CHECK:  TypeRef=MyBase:9:19 Extent=[12:18 - 12:24]
+};
+
+template<typename Q>
+struct Templ;
+
+template<typename MyBase>
+struct Derived3:  Templ<MyBase>::InnerIterator
+{
+    Derived3() : Templ<MyBase>::InnerIterator() {}
+// CHECK: TemplateRef=Templ:17:8 Extent=[22:18 - 22:23]
+// CHECK: TypeRef=MyBase:19:19 Extent=[22:24 - 22:30]
+};
+
+
+struct Outer {
+    template <typename Q>
+    struct Inner {
+        typedef Q Parm;
+    };
+};
+
+template<typename Q>
+struct Derived4:  Outer::Inner<Q>::Parm
+{
+    Derived4() : Outer::Inner<Q>::Parm() {}
+// CHECK: TypeRef=struct Outer:28:8 Extent=[38:18 - 38:23]
+// CHECK: TemplateRef=Inner:30:12 Extent=[38:25 - 38:30]
+// CHECK: TypeRef=Q:35:19 Extent=[38:31 - 38:32]
+};
+
+template<typename Q>
+struct Derived5:  Outer::Inner<Q>::Parm::InnerIterator
+{
+    Derived5() : Outer::Inner<Q>::Parm::InnerIterator() {}
+// CHECK: TypeRef=struct Outer:28:8 Extent=[47:18 - 47:23]
+// CHECK: TemplateRef=Inner:30:12 Extent=[47:25 - 47:30]
+// CHECK: TypeRef=Q:44:19 Extent=[47:31 - 47:32]
+};
+
+template<typename Q>
+struct Derived6:  Outer::Inner<Q>
+{
+    Derived6() : Outer::Inner<Q>() {}
+// CHECK: TypeRef=struct Outer:28:8 Extent=[56:18 - 56:23]
+// CHECK: TemplateRef=Inner:30:12 Extent=[56:25 - 56:30]
+// CHECK: TypeRef=Q:53:19 Extent=[56:31 - 56:32]
+};
+
+struct Base {};
+
+struct Derived7:  Outer::Inner<Base>::Parm
+{
+    Derived7() : Outer::Inner<Base>::Parm() {}
+// CHECK: TypeRef=struct Outer:28:8 Extent=[66:18 - 66:23]
+// CHECK: TemplateRef=Inner:30:12 Extent=[66:25 - 66:30]
+// CHECK: TypeRef=struct Base:62:8 Extent=[66:31 - 66:35]
+};
+
+struct Derived8:  Outer::Inner<Base>
+{
+    Derived8() : Outer::Inner<Base>() {}
+// CHECK: TypeRef=struct Outer:28:8 Extent=[74:18 - 74:23]
+// CHECK: TemplateRef=Inner:30:12 Extent=[74:25 - 74:30]
+// CHECK: TypeRef=struct Base:62:8 Extent=[74:31 - 74:35]
+};
+
+namespace Namespace {
+    template<typename Q> struct Templ;
+
+    struct Outer {
+        template <typename Q>
+        struct Inner {
+            typedef Q Parm;
+        };
+    };
+}
+
+template<typename MyBase>
+struct Derived9:  Namespace::Templ<MyBase>::InnerIterator
+{
+    Derived9() : Namespace::Templ<MyBase>::InnerIterator() {}
+// CHECK: NamespaceRef=Namespace:80:11 Extent=[94:18 - 94:27]
+// CHECK: TemplateRef=Templ:81:33 Extent=[94:29 - 94:34]
+// CHECK: TypeRef=MyBase:91:19 Extent=[94:35 - 94:41]
+};
+
+template<typename MyBase>
+struct Derived10:  Namespace::Templ<MyBase>
+{
+    Derived10() : Namespace::Templ<MyBase>() {}
+// CHECK: NamespaceRef=Namespace:80:11 Extent=[103:19 - 103:28]
+// CHECK: TemplateRef=Templ:81:33 Extent=[103:30 - 103:35]
+// CHECK: TypeRef=MyBase:100:19 Extent=[103:36 - 103:42]
+};
+
+template<typename MyBase>
+struct Derived11:  Namespace::Outer::Inner<MyBase>::Parm
+{
+    Derived11() : Namespace::Outer::Inner<MyBase>::Parm() {}
+// CHECK: NamespaceRef=Namespace:80:11 Extent=[112:19 - 112:28]
+// CHECK: TypeRef=struct Namespace::Outer:83:12 Extent=[112:30 - 112:35]
+// CHECK: TemplateRef=Inner:85:16 Extent=[112:37 - 112:42]
+// CHECK: TypeRef=MyBase:109:19 Extent=[112:43 - 112:49]
+};