Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Incorrect source range for instantiations of out-of-line defaulted special member functions of class templates #25682

Open Quuxplusone opened 8 years ago

Quuxplusone commented 8 years ago
Bugzilla Link PR25683
Status NEW
Importance P normal
Reported by Tim Prince (trprince@synopsys.com)
Reported on 2015-11-30 16:48:48 -0800
Last modified on 2019-07-02 10:26:38 -0700
Version 3.7
Hardware All All
CC llvm-bugs@lists.llvm.org, sig-rnd-sat-clang-bugs@synopsys.com, thonerma@synopsys.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
The following snippet produces an AST in which the starting source location
appears *after* the end location:

> $ cat bad.cpp
> template <typename> struct s { s(); };
> s<int> a;
> template <typename T> s<T>::s() = default;
> $ clang -v -std=c++11 -Xclang -ast-dump -fsyntax-only bad.cpp
> clang version 3.7.0 (tags/RELEASE_370/final)
> Target: x86_64-unknown-linux-gnu
> Thread model: posix
> Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6
> Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6.3
> Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6
> Candidate multilib: .;@m64
> Candidate multilib: 32;@m32
> Selected multilib: .;@m64
>  "/opt/pkg/clang-3.7.0/bin/clang-3.7" -cc1 -triple x86_64-unknown-linux-gnu -
fsyntax-only -disable-free -main-file-name bad.cpp -mrelocation-model static -
mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-
aliases -munwind-tables -target-cpu x86-64 -v -dwarf-column-info -resource-dir
/opt/pkg/clang-3.7.0/bin/../lib/clang/3.7.0 -internal-isystem
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6 -internal-isystem
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-gnu -
internal-isystem /usr/lib/gcc/x86_64-linux-
gnu/4.6/../../../../include/c++/4.6/backward -internal-isystem
/usr/local/include -internal-isystem /opt/pkg/clang-
3.7.0/bin/../lib/clang/3.7.0/include -internal-externc-isystem
/usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-
externc-isystem /usr/include -std=c++11 -fdeprecated-macro -fdebug-compilation-
dir /slowfs/sighome/trprince/lab/bz82561 -ferror-limit 19 -fmessage-length 211 -
mstackrealign -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-
show-option -fcolor-diagnostics -ast-dump -x c++ bad.cpp
> clang -cc1 version 3.7.0 based upon LLVM 3.7.0 default target x86_64-unknown-
linux-gnu
> ignoring nonexistent directory "/include"
> #include "..." search starts here:
> #include <...> search starts here:
>  /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6
>  /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-
gnu
>  /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/backward
>  /usr/local/include
>  /opt/pkg/clang-3.7.0/bin/../lib/clang/3.7.0/include
>  /usr/include/x86_64-linux-gnu
>  /usr/include
> End of search list.
> TranslationUnitDecl 0x86c6510 <<invalid sloc>> <invalid sloc>
> |-TypedefDecl 0x86c6a48 <<invalid sloc>> <invalid sloc> implicit __int128_t
'__int128'
> |-TypedefDecl 0x86c6aa8 <<invalid sloc>> <invalid sloc> implicit __uint128_t
'unsigned __int128'
> |-TypedefDecl 0x86c6e88 <<invalid sloc>> <invalid sloc> implicit
__builtin_va_list '__va_list_tag [1]'
> |-ClassTemplateDecl 0x86c7020 <bad.cpp:1:1, col:37> col:28 s
> | |-TemplateTypeParmDecl 0x86c6ed8 <col:11> col:11 typename
> | |-CXXRecordDecl 0x86c6f90 <col:21, col:37> col:28 struct s definition
> | | |-CXXRecordDecl 0x870f060 <col:21, col:28> col:28 implicit referenced
struct s
> | | `-CXXConstructorDecl 0x870f190 <col:32, col:34> col:32 s<type-parameter-0-
0> 'void (void)'
> | `-ClassTemplateSpecializationDecl 0x870f260 <col:1, col:37> col:28 struct s
definition
> |   |-TemplateArgument type 'int'
> |   |-CXXRecordDecl 0x870f508 prev 0x870f260 <col:21, col:28> col:28 implicit
struct s
> |   |-CXXConstructorDecl 0x870f5d8 <line:3:23, line:1:32> col:32 used
constexpr s 'void (void) noexcept'
> |   | `-CompoundStmt 0x870feb0 <col:32>
> |   |-CXXConstructorDecl 0x870f6d8 <col:28> col:28 implicit constexpr s 'void
(const struct s<int> &)' inline noexcept-unevaluated 0x870f6d8
> |   | `-ParmVarDecl 0x870f820 <col:28> col:28 'const struct s<int> &'
> |   `-CXXConstructorDecl 0x870f8b8 <col:28> col:28 implicit constexpr s 'void
(struct s<int> &&)' inline noexcept-unevaluated 0x870f8b8
> |     `-ParmVarDecl 0x870fa00 <col:28> col:28 'struct s<int> &&'
> |-VarDecl 0x870f408 <line:2:1, col:8> col:8 a 's<int>':'struct s<int>'
callinit
> | `-CXXConstructExpr 0x870fa68 <col:8> 's<int>':'struct s<int>' 'void (void)
noexcept'
> `-CXXConstructorDecl 0x870fd40 parent 0x86c6f90 prev 0x870f190 <line:3:1,
col:31> col:29 s<type-parameter-0-0> 'void (void)'
> $

In particular, note the line numbers on the second CXXConstructorDecl:
> CXXConstructorDecl 0x870f5d8 <line:3:23, line:1:32>

This doesn't seem to cause any problems in clang proper, but passing this range
to clang::PreprocessingRecord::getPreprocessedEntitiesInRange triggers an
assertion, since the start of the range occurs after the end.
Quuxplusone commented 5 years ago
I updated the summary as this issue occurs for all instantiated out-of-line
defaulted special member function definitions, not just constructors.  It does
not occur for non-defaulted definitions (the following test case includes a
member function for comparison purposes):

$ cat t.cpp
template<typename T>
struct S {
  S();
  S(const S&);
  S(S&&);
  ~S();
  S& operator=(const S&);
  S& operator=(S&&);
  void mf();
  T dm;
};
template<typename T>
S<T>::S() = default;
template<typename T>
S<T>::S(const S<T>&) = default;
template<typename T>
S<T>::S(S<T>&&) = default;
template<typename T>
S<T>::~S() = default;
template<typename T>
S<T>& S<T>::operator=(const S<T>&) = default;
template<typename T>
S<T>& S<T>::operator=(S<T>&&) = default;
template<typename T>
void S<T>::mf() {}
void instantiate_all_the_things() {
  S<int> s1;
  S<int> s2(s1);
  S<int> s3(static_cast<S<int>&&>(s2));
  s2 = s1;
  s3 = static_cast<S<int>&&>(s2);
  s3.mf();
}

$ clang --version
clang version 8.0.0 (tags/RELEASE_800/final)
Target: x86_64-unknown-linux-gnu
...

$ clang -c -Xclang -ast-dump t.cpp
TranslationUnitDecl 0x5efc378 <<invalid sloc>> <invalid sloc>
...
|-ClassTemplateDecl 0x5f38368 <t.cpp:1:1, line:11:1> line:2:8 S
...
| `-ClassTemplateSpecializationDecl 0x5f69b00 <line:1:1, line:11:1> line:2:8
struct S definition
...
|   |-CXXConstructorDecl 0x5f6a660 <line:13:1, line:3:5> col:3 used S 'void ()
noexcept' default
...
|   |-CXXConstructorDecl 0x5f6a828 <line:15:1, line:4:13> col:3 used constexpr
S 'void (const S<int> &) noexcept' default
...
|   |-CXXConstructorDecl 0x5f6a9f8 <line:17:1, line:5:8> col:3 used constexpr S
'void (S<int> &&) noexcept' default
...
|   |-CXXDestructorDecl 0x5f6aad8 <line:19:1, line:6:6> col:3 used ~S 'void ()
noexcept' default
...
|   |-CXXMethodDecl 0x5f6acb8 <line:21:1, line:7:24> col:6 used operator=
'S<int> &(const S<int> &) noexcept' default
...
|   |-CXXMethodDecl 0x5f6ae38 <line:23:1, line:8:19> col:6 used operator=
'S<int> &(S<int> &&) noexcept' default
...
|   |-CXXMethodDecl 0x5f6aee8 <line:25:1, col:18> line:9:8 used mf 'void ()'
...

For each of the special member function declarations within the instantiated
class template specialization, note that the begin location corresponds to the
out-of-line definition, but that the end location corresponds to the in-class
declaration.  The instantiation of the 'mf' member function illustrates the
expected behavior.
Quuxplusone commented 5 years ago
Debugging revelations from the following minimal test case:

template<typename>
struct S {
  S();
};
template<typename T>
S<T>::S() = default;
S<int> s;

With a break point set on clang::CXXConstructorDecl::Create(), a number of
constructions of CXXConstructorDecl are observed.  The first is for the in-
class declaration at line 3.  The second is for the out-of-line definition on
lines 5-6.  The third is for the instantiation of the constructor for the
C<int> specialization; the case this bug is concerned with.

That third instance is created here:

lib/Sema/SemaTemplateInstantiateDecl.cpp:
1903 Decl *
1904 TemplateDeclInstantiator::VisitCXXMethodDecl(CXXMethodDecl *D,
1905                                       TemplateParameterList
*TemplateParams,
1906                                       bool IsClassScopeSpecialization) {
....
1983   SourceLocation StartLoc = D->getInnerLocStart();
....
1986   if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(D)) {
1987     Method = CXXConstructorDecl::Create(SemaRef.Context, Record,
1988                                         StartLoc, NameInfo, T, TInfo,
1989                                         Constructor->isExplicit(),
1990                                         Constructor->isInlineSpecified(),
1991                                         false, Constructor->isConstexpr());
1992     Method->setRangeEnd(Constructor->getEndLoc());
....
2196 }

Debugging revealed that 'D' corresponds to the first constructed
CXXConstructorDecl object corresponding to the in-class declaration.  Note that
the provided 'StartLoc' is obtained from 'getInnerLocStart()' called for the in-
class declaration.  Since the in-class declaration does not have a template
parameter list, this corresponds to the start location of that declaration,
line 3:3.  The end location is then set to the end location of that declaration
at line 3:5, making the locations consistent.

What ends up happening is that a call to 'setInnerLocStart()' is made here:

lib/Sema/SemaTemplateInstantiateDecl.cpp:
3890 void Sema::InstantiateFunctionDefinition(SourceLocation
PointOfInstantiation,
3891                                          FunctionDecl *Function,
3892                                          bool Recursive,
3893                                          bool DefinitionRequired,
3894                                          bool AtEndOfTU) {
....
4018   // Copy the inner loc start from the pattern.
4019   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
....
4100 }

Lines 4018-4019 above were added via the change in the following link to
correct  source locations for such instantiations.
- https://github.com/llvm/llvm-
project/commit/12dcbf3eaa6d2c8b9ee814ddb8bf23bef644bfaf

The call to setInnerLocStart() changes the perceived beginning of the function
(since, for this declaration, there is no template parameter list).  Within the
debugger, calling 'Function->dump()' before and after the call to
'setInnerLocStart()' demonstrates the change:

(gdb) p Function->dump()
CXXConstructorDecl 0xae44e88 <m1.cpp:3:3, col:5> col:3 used S 'void ()'
...
(gdb) p Function->dump()
CXXConstructorDecl 0xae44e88 <m1.cpp:6:1, line:3:5> col:3 used S 'void ()'

The same inconsistent location issue occurs here for non-defaulted functions as
well.  However, for functions with bodies, the end location gets corrected
later by a call to 'setBody()' from 'Sema::ActOnFinishFunctionBody()':

lib/AST/Decl.cpp:
2741 void FunctionDecl::setBody(Stmt *B) {
2742   Body = B;
2743   if (B)
2744     EndRangeLoc = B->getEndLoc();
2745 }

However, even in this case, the non-inner begin location is never updated with
the result being:
  Decl::Loc = begin location of the in-class declaration.
  DeclaratorDecl::InnerLocStart = inner location of the out-of-line definition.
  FunctionDecl::EndRangeLoc = end location of the out-of-line definition.

I'm not sure what the ramifications are of the begin and inner-begin locations
being out of sync like this.

The following change appears to suffice to work around this issue:

$ git diff
diff --git a/lib/Sema/SemaTemplateInstantiateDecl.cpp
b/lib/Sema/SemaTemplateInstantiateDecl.cpp
index e76a34c..a07742d 100644
--- a/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4015,8 +4015,10 @@ void Sema::InstantiateFunctionDefinition(SourceLocation
PointOfInstantiation,
   // unimported module.
   Function->setVisibleDespiteOwningModule();

-  // Copy the inner loc start from the pattern.
+  // Copy source locations from the pattern.
+  Function->setLocation(PatternDecl->getBeginLoc());
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
+  Function->setRangeEnd(PatternDecl->getEndLoc());

   EnterExpressionEvaluationContext EvalContext(
       *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);

With the above change, I get 9 test failures in the Clang test suite.  I
haven't yet looked at the details of those failures.

Failing Tests (9):
    Clang :: Index/cursor-ref-names.cpp
    Clang :: Index/preamble_macro_template.cpp
    Clang :: Index/recursive-cxx-member-calls.cpp
    Clang :: SemaCXX/member-init.cpp
    Clang :: SemaTemplate/virtual-member-functions.cpp
    Clang :: Templight/templight-deduced-func.cpp
    Clang :: Templight/templight-default-func-arg.cpp
    Clang :: Templight/templight-exception-spec-func.cpp
    Clang :: Templight/templight-explicit-template-arg.cpp

  Expected Passes    : 42326
  Expected Failures  : 170
  Unsupported Tests  : 679
  Unexpected Failures: 9
Quuxplusone commented 5 years ago
A proposed fix has been posted for review:
- https://reviews.llvm.org/D64087