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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
