[lldb] CXXRecordDecl::IsStandardLayout incorrectly unset when importing a class with a single base class

[Michael137]

Essentially it’s just about importing a type with a single base class with member fields. There isn't anything problematic I ran into with this per-se, but it was a discrepancy I noticed while working on a different issue.

Setup:

// lib.h
class Base {
  void* Field;
};

class Derived : Base {};

struct Foo {
  Derived d;
};

// main.cpp
#include "lib.h"

int main() {
  Foo f;

  return 0;
}

./bin/lldb \
    -o "br se -p return -X main" \      
    -o "log enable lldb ast" \          
    -o "r" \                            
    -o "p f" \                          
    -o "q" \                            
    a.out                               

This will print:

-CXXRecordDecl 0x159f31d10 <<invalid sloc>> <invalid sloc> <undeserialized declarations> class Foo definition
 |-DefinitionData pass_in_registers trivially_copyable trivial literal
 | |-DefaultConstructor exists trivial needs_implicit                                                   
 | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 | |-MoveConstructor exists simple trivial needs_implicit                                               
 | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
 | |-MoveAssignment exists simple trivial needs_implicit                                                
 | `-Destructor simple irrelevant trivial needs_implicit                                                
 `-FieldDecl 0x159f32180 <<invalid sloc>> <invalid sloc> d 'test::Derived'
-CXXRecordDecl 0x159f31e50 <<invalid sloc>> <invalid sloc> class Derived definition
 |-DefinitionData pass_in_registers trivially_copyable trivial literal
 | |-DefaultConstructor exists trivial needs_implicit                                                   
 | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 | |-MoveConstructor exists simple trivial needs_implicit                                               
 | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
 | |-MoveAssignment exists simple trivial needs_implicit                                                
 | `-Destructor simple irrelevant trivial needs_implicit                                                
 `-private 'test::Base'                           
-CXXRecordDecl 0x159f31f90 <<invalid sloc>> <invalid sloc> class Base definition
 |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial literal
 | |-DefaultConstructor exists trivial needs_implicit                                                   
 | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 | |-MoveConstructor exists simple trivial needs_implicit                                               
 | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
 | |-MoveAssignment exists simple trivial needs_implicit                                                
 | `-Destructor simple irrelevant trivial needs_implicit                                                
 `-FieldDecl 0x159f32110 <<invalid sloc>> <invalid sloc> Mem 'void *'

Note that both class Derived and class Foo don’t have standard_layout set in its DefinitionData despite class Base having it set.

According to the standard, a standard-layout class is allowed to have a single base class with member fields (e.g., https://godbolt.org/z/a467evfdx). This is codified in RecordDecl::setBases as follows:

…
// C++2a [class]p7:                                                     
//   A standard-layout class is a class that:                           
//    [...]                                                             
//    -- has all non-static data members and bit-fields in the class and
//       its base classes first declared in the same class              
if (BaseClassDecl->data().HasBasesWithFields ||                         
    !BaseClassDecl->field_empty()) {                                    
  if (data().HasBasesWithFields) {
    data().IsStandardLayout = false;                     
  }                                                                     
  data().HasBasesWithFields = true;                                     
}                                                        
…               

When we're asked to complete Foo we call into DWARFASTParserClang::CompleteRecordType and then TypeSystemClang::TransferBaseClasses which calls CXXRecordDecl::setBases.

When we then import the definition into the scratch AST (see ASTNodeImporter::ImportDefinition), we do:

struct CXXRecordDecl::DefinitionData &ToData = ToCXX->data();    
struct CXXRecordDecl::DefinitionData &FromData = FromCXX->data();
#define FIELD(Name, Width, Merge) \                 
ToData.Name = FromData.Name;                        
#include "clang/AST/CXXRecordDeclDefinitionBits.def"

if (!Bases.empty())                           
  ToCXX->setBases(Bases.data(), Bases.size());

So now the To decl has HasBasesWithFields set going into CXXRecordDecl::setBases; this unsets IsStandardLayout because we mistakenly claimed that To has more than one base class with members.

[llvmbot]

@llvm/issue-subscribers-lldb

[Michael137]

Actually this seems like an ASTImporter bug. Running -ast-merge on the examples results in an AST doesn’t correctly attach the standard_layout attribute to Derived and Foo.