Page MenuHomePhabricator

[CrossTU] Handle case when no USR could be generated during Decl search.
ClosedPublic

Authored by balazske on Jul 30 2019, 6:18 AM.

Details

Summary

When searching for a declaration to be loaded the "lookup name" for every
other Decl is computed. If the USR can not be determined here should be
not an assert, instead skip this Decl.

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Jul 30 2019, 6:18 AM

Thank you for this patch Balazs! Could you please elaborate in which cases we cannot generate the USR? Does it happen only if we have an anonymous union inside a function? Are there no other cases?

It looks like that the problem can happen when the anonymous union is in any DeclContext and for CTU import the import of a variable is requested and that variable is in a related DeclContext (it can be at upper or lower level). (See code of findDefInDeclContext: If used for variables, it goes through every DeclContext and computes USR for specific VarDecls until the one is found. If we have luck it may be found before the union. An anonymous union can not be imported because we get no USR for it so it is OK to ignore these.) So the test can be written in many other forms but this case (the code of f) was encountered in protobuf code.

It looks like that the problem can happen when the anonymous union is in any DeclContext and for CTU import the import of a variable is requested and that variable is in a related DeclContext (it can be at upper or lower level). (See code of findDefInDeclContext: If used for variables, it goes through every DeclContext and computes USR for specific VarDecls until the one is found. If we have luck it may be found before the union. An anonymous union can not be imported because we get no USR for it so it is OK to ignore these.) So the test can be written in many other forms but this case (the code of f) was encountered in protobuf code.

Thanks for the explanation.

So, IIUC we cannot generate the USR for the member of the anonymous union, right? f or i below does not have a USR.

class TestAnonUnionUSR {
public:
  inline float f(int value) {
    union {
      float f;
      int i;
    };
    i = value;
    return f;
  }
  static const int Test;
};

And findDefInDeclContext goes through all children DC. So if the DC is TestAnonUnionUSR then we may try to get the USR for f and i. Please confirm if I understand it correctly or not.

Probably this is a problem case too but only if the f or i has an initializer expression and really no USR is generated for f or i. But what I have found is that there is a VarDecl for a "invisible" variable whose type is the anonymous union and it has a (default) initializer too. If the search reaches DC of the f it will try to get the USR for this variable.

Probably this is a problem case too but only if the f or i has an initializer expression and really no USR is generated for f or i. But what I have found is that there is a VarDecl for a "invisible" variable whose type is the anonymous union and it has a (default) initializer too. If the search reaches DC of the f it will try to get the USR for this variable.

OK. Just for the record and for later reference, I copy the test code and the AST here.

class TestAnonUnionUSR {
public:
  inline float f(int value) {
    union {
      float f;
      int i;
    };
    i = value;
    return f;
  }
  static const int Test;
};
`-CXXRecordDecl 0xdb9fe0 </tmp/usr.cpp:1:1, line:12:1> line:1:7 class TestAnonUnionUSR definition
  |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-CXXRecordDecl 0xdba0f8 <col:1, col:7> col:7 implicit class TestAnonUnionUSR
  |-AccessSpecDecl 0xdba188 <line:2:1, col:7> col:1 public
  |-CXXMethodDecl 0xdba2c0 <line:3:3, line:10:3> line:3:16 f 'float (int)' inline
  | |-ParmVarDecl 0xdba1c8 <col:18, col:22> col:22 used value 'int'
  | `-CompoundStmt 0xdbae70 <col:29, line:10:3>
  |   |-DeclStmt 0xdbac68 <line:4:5, line:7:6>
  |   | |-CXXRecordDecl 0xdba400 <line:4:5, line:7:5> line:4:5 union definition
  |   | | |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal has_variant_members
  |   | | | |-DefaultConstructor exists trivial
  |   | | | |-CopyConstructor simple trivial has_const_param implicit_has_const_param
  |   | | | |-MoveConstructor exists simple trivial
  |   | | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  |   | | | |-MoveAssignment exists simple trivial needs_implicit
  |   | | | `-Destructor simple irrelevant trivial needs_implicit
  |   | | |-FieldDecl 0xdba530 <line:5:7, col:13> col:13 referenced f 'float'
  |   | | |-FieldDecl 0xdba598 <line:6:7, col:11> col:11 referenced i 'int'
  |   | | |-CXXConstructorDecl 0xdba6d8 <line:4:5> col:5 implicit used  'void () noexcept' inline default trivial
  |   | | | `-CompoundStmt 0xdbab68 <col:5>
  |   | | |-CXXConstructorDecl 0xdba808 <col:5> col:5 implicit constexpr  'void (const (anonymous union at /tmp/usr.cpp:4:5) &)' inline default trivial noexcept-unevaluated 0xdba808
  |   | | | `-ParmVarDecl 0xdba918 <col:5> col:5 'const (anonymous union at /tmp/usr.cpp:4:5) &'
  |   | | `-CXXConstructorDecl 0xdba9b8 <col:5> col:5 implicit constexpr  'void ((anonymous union at /tmp/usr.cpp:4:5) &&)' inline default trivial noexcept-unevaluated 0xdba9b8
  |   | |   `-ParmVarDecl 0xdbaac8 <col:5> col:5 '(anonymous union at /tmp/usr.cpp:4:5) &&'
  |   | `-VarDecl 0xdba658 <col:5> col:5 implicit used '(anonymous union at /tmp/usr.cpp:4:5)' callinit
  |   |   `-CXXConstructExpr 0xdbab78 <col:5> '(anonymous union at /tmp/usr.cpp:4:5)' 'void () noexcept'
  |   |-BinaryOperator 0xdbad08 <line:8:5, col:9> 'int' lvalue '='
  |   | |-MemberExpr 0xdbaca0 <col:5> 'int' lvalue .i 0xdba598
  |   | | `-DeclRefExpr 0xdbac80 <col:5> '(anonymous union at /tmp/usr.cpp:4:5)' lvalue Var 0xdba658 '' '(anonymous union at /tmp/usr.cpp:4:5)'
  |   | `-ImplicitCastExpr 0xdbacf0 <col:9> 'int' <LValueToRValue>
  |   |   `-DeclRefExpr 0xdbacd0 <col:9> 'int' lvalue ParmVar 0xdba1c8 'value' 'int'
  |   `-ReturnStmt 0xdbae60 <line:9:5, col:12>
  |     `-ImplicitCastExpr 0xdbae48 <col:12> 'float' <LValueToRValue>
  |       `-MemberExpr 0xdbae18 <col:12> 'float' lvalue .f 0xdba530
  |         `-DeclRefExpr 0xdbadf8 <col:12> '(anonymous union at /tmp/usr.cpp:4:5)' lvalue Var 0xdba658 '' '(anonymous union at /tmp/usr.cpp:4:5)'
  `-VarDecl 0xdba380 <line:11:3, col:20> col:20 Test 'const int' static
martong accepted this revision.Aug 1 2019, 2:33 AM

LGTM! Please proceed and commit! :)

This revision is now accepted and ready to land.Aug 1 2019, 2:33 AM
balazske updated this revision to Diff 212986.Aug 2 2019, 1:24 AM

Rebase and add a new testing case.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 5:10 AM