martong 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();
----------------
labath wrote:
> shafik wrote:
> > 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.
> I've been looking at this code, but I'm still not very familiar with it, so
> what I am asking may be obvious, but... What is the expected behavior for
> non-minimal imports for code like this?
> ```
> struct A { ...};
> struct B { A* a; }; // Note the pointer
> ```
> Should importing B also import the definition of the A struct ? As I think
> that should not happen in the minimal import... If we get rid of the minimal
> flag, and rely solely on argument passing, we will need to be careful, as it
> shouldn't be passed _everywhere_ (it should stop at pointers for instance).
> But then it may not be possible to reproduce the current non-minimal import,
> as it (I think) expects that A will be fully imported too...
>
> > I don't believe that should have other fields where we could have a record
> > that effects the layout
> This isn't exactly layout related, but there is the question of covariant
> methods. If a method is covariant, then its return type must be complete.
> Currently we handle the completion of these in LLDB, but that solution is a
> bit hacky. It might be interesting if that could be handled by the ast
> importer as well.
> What is the expected behavior for non-minimal imports for code like this?
In this case we import the definition of A fully, even though that seems
unnecessary. This may be an existing technical debt in
clang::ASTNodeImporter::VisitPointerType. Perhaps instead of a full import we
could just do a minimal import for the PointeeType always. Similarly to
references. But I am not sure about whether we would know during a subsequent
import (where the full type is really needed) that the type had been minimally
imported before.
Actually, we had a similar discussion with Jaroslav about this before:
http://lists.llvm.org/pipermail/lldb-dev/2019-November/015738.html
> If we get rid of the minimal flag, and rely solely on argument passing, we
> will need to be careful as it shouldn't be passed _everywhere_ (it should
> stop at pointers for instance)
Yes, I agree. Currently, the problem is that even though the flag is set, we
require complete imports via the public ImportDefinition function. And during
this supposedly full import we sometimes make import decisions based on the
flag, ouch. Perhaps we could keep the flag, but in case of the
ASTImporter::ImportDefinition we should set that to false at entry and before
returning we should set that back to true .(?)
Anyhow, I really think we shall have only one mechanism to decide whether we
want a full or just a partial import, this should be either the flag or the
Kind.
> This isn't exactly layout related, but there is the question of covariant
> methods. If a method is covariant, then its return type must be complete.
We already import all the base classes of a derived class (at least in full
import). But, we do not import all the derived classes during the import of the
base. Is this what you do in LLDB to support covariant return types?
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
