This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Serge_Preis on Apr 24 2017, 9:45 AM.

Details

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().

Diff Detail

Event Timeline

Serge_Preis created this revision.Apr 24 2017, 9:45 AM

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

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.

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.

What do you think?

Seems reasonable.

Serge_Preis updated this revision to Diff 96752.EditedApr 26 2017, 8:38 AM
  • 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

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.

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 edited edge metadata.Jun 21 2017, 8:31 AM

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!

Fixed review comments:

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

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 accepted this revision.Jun 23 2017, 5:53 AM

This LGTM. I will commit it for you.

This revision is now accepted and ready to land.Jun 23 2017, 5:53 AM
This revision was automatically updated to reflect the committed changes.

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.

I reverted it in r306111 for now.

Serge_Preis added a comment.EditedJun 26 2017, 1:48 AM

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.

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

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.

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.

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 added a comment.EditedJun 27 2017, 1:01 AM

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.

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

Thanks! I'll recommit today.

Recommitted in r306392.