[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik marked 2 inline comments as done.
shafik added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+          QualType FromTy = ArrayFrom->getElementType();
+          QualType ToTy = ArrayTo->getElementType();
+
+          FromRecordDecl = FromTy->getAsRecordDecl();
+          ToRecordDecl = ToTy->getAsRecordDecl();
----------------
martong wrote:
> labath wrote:
> > What about 2- or n-dimensional arrays?
> @labath, this is a very good question! And made me realize that D71378 is fundamentally flawed (@shafik, please take no offense). Let me explain.
> 
> So, with D71378, we added the `if (ImportedOrErr) { ... }` block to import definitions specifically of fields' Record types. But we forget to handle arrays. Now we may forget to handle multidimensional arrays ... and we may forget to handle other language constructs. So, we would finally end up in re-implementing the logic of `ASTNodeImporter::VisitFieldDecl`.
> 
> So all this should have been handled properly by the preceding import call of the FieldDecl! Here
> ```
> 1735: ExpectedDecl ImportedOrErr = import(From);
> ```
> I have a suspicion that real reason why this import call fails in case of the public ASTImporter::ImportDefinition() is that we fail to drive through the import kind (`IDK_Everything`) during the import process.
> Below we set IDK_Everything and start a complete import process.
> ```
>   8784   if (auto *ToRecord = dyn_cast<RecordDecl>(To)) {
>   8785     if (!ToRecord->getDefinition()) {
>   8786       return Importer.ImportDefinition(   // ASTNodeImporter::ImportDefinition !
>   8787           cast<RecordDecl>(FromDC), ToRecord,
>   8788           ASTNodeImporter::IDK_Everything);
>   8789     }
>   8790   }
> ```
> However, there may be many places where we fail to channel through that we want to do a complete import. E.g here
> ```
> 1957           ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
> ```
> we do another definition import and IDK_Everything is not set. So we may have a wrong kind of import since the "minimal" flag is set.
> 
> The thing is, it is really confusing and error-prone to have both the `ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to be in contradiction to each other some times.
> I think we should get rid of the Minimal flag. And Kind should be either a full completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd scrap the IDK_Default too. Alternatively we could have a Kind member in AST**//Node//**Importer.
> I think we should go into this direction to avoid similar problems during CodeGen in the future. WDYT?
No offense, you definitely understand the Importer better than I, so I value your input here. I don't believe that should have other fields where we could have a record that effects the layout but the concern is still valid and yes I did miss multi-dimensional arrays.

Your theory on `IDK_Everything` not be pushed through everywhere, I did a quick look and it seems pretty reasonable. 

I agree that the `ASTImporter::Minimal` flag and the `ImportDefinitionKind` seem to inconsistent or perhaps a work-around. That seems like a bigger change with a lot more impact beyond this fix but worth looking into longer term. 

If pushing through `IDK_Everything` everywhere fixes the underlying issue I am very happy to take that approach. If not we can discuss alternatives. 


================
Comment at: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp:22-27
+  union {
+    struct {
+      unsigned char *_s;
+    } t;
+    char *tt[1];
+  } U;
----------------
labath wrote:
> What's the significance of this union?
Not sure, I was not able to discern why this is important to reproduce the crash. I wanted feedback on the approach so I left to further investigation later on. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86660/new/

https://reviews.llvm.org/D86660