Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

[shafik]

Summary

Once we start the definition of an ObjCInterfaceDecl we won't attempt to ImportDeclContext later on. Unlike RecordDecl case which uses DefinitionCompleter to force completeDefinition we don't seem to have a similar mechanism for ObjCInterfaceDecl.

This fix was needed due to a bug we see in LLDB expression parsing where an initial expression cause an ObjCInterfaceDecl to be defined and subsequent expressions during import do not call ImportDeclContext and we can end up in a situation where ivars are imported out of order and not all ivars are imported e.g.

(lldb) expr chb1->hb->field2
ObjCInterfaceDecl 0x7f9457495fe0 <<invalid sloc>> <invalid sloc> <undeserialized declarations> HasBitfield1
|-super ObjCInterface 0x7f9457495e78 'NSObject'
`-ObjCIvarDecl 0x7f945749d478 <<invalid sloc>> <invalid sloc> field2 'unsigned int' public
  `-IntegerLiteral 0x7f945749d458 <<invalid sloc>> 'int' 1

We have field2 which is the second ivar but we are missing field1.

In this particular case ASTContext::lookupFieldBitOffset(...) assumes we have all ivars and they are in order to obtain the index of the ivar and we break this assumption. This leads eventually to bad IRGen since it has the wrong field offset.

[shafik]

Updating diff since the parent landed.

[martong]

Unlike RecordDecl case which uses DefinitionCompleter to force completeDefinition we don't seem to have a similar mechanism for ObjCInterfaceDecl.

It is unfortunate that the AST does not have something similar for ObjC interfaces.
So, this seems to be the right way unless we want to modify DeclObjC.h.
LGTM!

clang/lib/AST/ASTImporter.cpp

Error ASTNodeImporter::ImportDefinition(
   if (From->getImplementation()) {
     if (Expected<ObjCImplementationDecl *> ToImplOrErr =
         import(From->getImplementation()))
       To->setImplementation(*ToImplOrErr);
     else
       return ToImplOrErr.takeError();
   }
 
-  if (shouldForceImportDeclContext(Kind)) {
-    // Import all of the members of this class.
-    if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
-      return Err;
-  }
+
   return Error::success();
 }
 
 Expected<ObjCTypeParamList *>
 ASTNodeImporter::ImportObjCTypeParamList(ObjCTypeParamList *list) {
   if (!list)
     return nullptr;
 

lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py

 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 class TestBitfieldIvars(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
 
     @skipUnlessDarwin
     def test(self):
         self.build()
         lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.m"))
 
         self.expect_expr("chb->hb->field1", result_type="unsigned int", result_value="0")
-
-        ## FIXME field2 should have a value of 1
-        self.expect("expr chb->hb->field2", matching=False, substrs = ["= 1"]) # this must happen second
+        ## This should happen second
+        self.expect_expr("chb->hb->field2", result_type="unsigned int", result_value="1")
 
         self.expect_expr("hb2->field1", result_type="unsigned int", result_value="10")
         self.expect_expr("hb2->field2", result_type="unsigned int", result_value="3")
         self.expect_expr("hb2->field3", result_type="unsigned int", result_value="4")
 
         self.expect("frame var *hb2", substrs = [ 'x =', '100',
                                              'field1 =', '10',
                                              'field2 =', '3',