Recombining AST Nodes

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Recombining AST Nodes

Deep Majumder via cfe-dev
I'm in the process of debugging (again) issues with anonymous struct
decls.  I have the following basic structures located in a C source
file that I'm attempting to transform.  I want to preserve the layout
of the structs.

struct{
  int A;
}S;

struct{
  int B;
  struct{
    int C;
  }T;
}R;

When I use my AST visitors, I can pick up the RecordDecl for the
struct definitions and the VarDecl for the variable definitions, but
the transformed code is outputted as:
struct{
  int A;
};
struct (anonymous) S;

I can view the RecordDecl as as TagDecl and save off a reference to it
if it is !isFreeStanding(), but how does one go about recombining the
declaration of "S" and the original TagDecl?

For the VarDecl ("S" in this case), I can determine whether or not its
an "ElaboratedType", but I'm not sure how to utilize the saved
reference to the RecordDecl:

if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
   /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
}
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
Hi John,

IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):


You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.

To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)

Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.

Hope that works — good luck,

Dave

On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:

I'm in the process of debugging (again) issues with anonymous struct
decls.  I have the following basic structures located in a C source
file that I'm attempting to transform.  I want to preserve the layout
of the structs.

struct{
 int A;
}S;

struct{
 int B;
 struct{
   int C;
 }T;
}R;

When I use my AST visitors, I can pick up the RecordDecl for the
struct definitions and the VarDecl for the variable definitions, but
the transformed code is outputted as:
struct{
 int A;
};
struct (anonymous) S;

I can view the RecordDecl as as TagDecl and save off a reference to it
if it is !isFreeStanding(), but how does one go about recombining the
declaration of "S" and the original TagDecl?

For the VarDecl ("S" in this case), I can determine whether or not its
an "ElaboratedType", but I'm not sure how to utilize the saved
reference to the RecordDecl:

if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
  /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
}
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
David, thanks for the response.  Please excuse my ignorance, but I'm a
bit confused as to the order this needs to occur.  I'll try to
reiterate what you said as follows:

1. If a TagDecl that is !isFreeStanding() is found, create a new
RecordDecl to represent it and add it to the DeclContext
2. Save a reference to the newly created RecordDecl
3. The next FieldDecl whose TagDecl matches the new RecordDecl should
be found; at this point create a new ElaboratedType using that TagDecl
4. Create a new FieldDecl with the new ElaboratedType as the type

Is this the general process?

On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:

>
> Hi John,
>
> IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):
>
> https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410
>
> You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.
>
> To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)
>
> Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.
>
> Hope that works — good luck,
>
> Dave
>
> On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:
>
> I'm in the process of debugging (again) issues with anonymous struct
> decls.  I have the following basic structures located in a C source
> file that I'm attempting to transform.  I want to preserve the layout
> of the structs.
>
> struct{
>  int A;
> }S;
>
> struct{
>  int B;
>  struct{
>    int C;
>  }T;
> }R;
>
> When I use my AST visitors, I can pick up the RecordDecl for the
> struct definitions and the VarDecl for the variable definitions, but
> the transformed code is outputted as:
> struct{
>  int A;
> };
> struct (anonymous) S;
>
> I can view the RecordDecl as as TagDecl and save off a reference to it
> if it is !isFreeStanding(), but how does one go about recombining the
> declaration of "S" and the original TagDecl?
>
> For the VarDecl ("S" in this case), I can determine whether or not its
> an "ElaboratedType", but I'm not sure how to utilize the saved
> reference to the RecordDecl:
>
> if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
>   /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
> }
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
No that is not a general process — I assumed you had modified the AST by replacing the existing RecordDecl with a new one, and were printing the whole AST, due to the behavior you are getting, and given your description of your visitors as "transforming" the code, which RecursiveASTVisitors don’t do by default — they just visit it, not modifying anything.  (Unless you're overriding TreeTransformer instead?)

But you might also be getting this behavior if you are manually printing each Decl you want, in a RecursiveASTVisitor.  If this is the case, remember, struct { int i } s; is two distinct Decls, and that loop in DeclPrinter::VisitDeclContext that I linked to is necessary to get the proper printing behavior for such cases.

There’s not quite enough information to know exactly what you need to do, but let’s start with the advice that that if you are manually printing individual Decls (e.g. D->print(OS)), that won’t work for your case — you need to instead handle a DeclContext as a whole and perhaps just copy and paste the important parts of DeclPrinter::VisitDeclContext and other parts of DeclPrinter.  If you’re still confused share a few more details — are you using RecursiveASTVisitor, are you printing from it, do you just want to print an output file etc.

Good luck,

Dave

> On Feb 5, 2021, at 2:18 PM, John Leidel <[hidden email]> wrote:
>
> David, thanks for the response.  Please excuse my ignorance, but I'm a
> bit confused as to the order this needs to occur.  I'll try to
> reiterate what you said as follows:
>
> 1. If a TagDecl that is !isFreeStanding() is found, create a new
> RecordDecl to represent it and add it to the DeclContext
> 2. Save a reference to the newly created RecordDecl
> 3. The next FieldDecl whose TagDecl matches the new RecordDecl should
> be found; at this point create a new ElaboratedType using that TagDecl
> 4. Create a new FieldDecl with the new ElaboratedType as the type
>
> Is this the general process?
>
> On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:
>>
>> Hi John,
>>
>> IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):
>>
>> https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410
>>
>> You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.
>>
>> To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)
>>
>> Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.
>>
>> Hope that works — good luck,
>>
>> Dave
>>
>> On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:
>>
>> I'm in the process of debugging (again) issues with anonymous struct
>> decls.  I have the following basic structures located in a C source
>> file that I'm attempting to transform.  I want to preserve the layout
>> of the structs.
>>
>> struct{
>> int A;
>> }S;
>>
>> struct{
>> int B;
>> struct{
>>   int C;
>> }T;
>> }R;
>>
>> When I use my AST visitors, I can pick up the RecordDecl for the
>> struct definitions and the VarDecl for the variable definitions, but
>> the transformed code is outputted as:
>> struct{
>> int A;
>> };
>> struct (anonymous) S;
>>
>> I can view the RecordDecl as as TagDecl and save off a reference to it
>> if it is !isFreeStanding(), but how does one go about recombining the
>> declaration of "S" and the original TagDecl?
>>
>> For the VarDecl ("S" in this case), I can determine whether or not its
>> an "ElaboratedType", but I'm not sure how to utilize the saved
>> reference to the RecordDecl:
>>
>> if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
>>  /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
>> }
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
David, thanks again for the response.  We are indeed transforming the
AST using combinations of new Decl's via Create* and tree transforms.
This is code we're helping to maintain (not the original authors), so
its a bit disjunct.

The original code is here if you're interested:
https://github.com/Intrepid/upc2c/blob/master/Transform.cpp#L2064

Basically, in the FieldDecl iterator loop, the RecordDecl's for the
struct definition are stored in the new DeclContext, but the
FieldDecl's are recreated.  I'm curious whether we check for an
ElaboratedType and if it exists for the target FieldDecl, utilize the
existing Decl for the new DeclContext.

Thoughts?

best
john


On Fri, Feb 5, 2021 at 3:23 PM David Rector <[hidden email]> wrote:

>
> No that is not a general process — I assumed you had modified the AST by replacing the existing RecordDecl with a new one, and were printing the whole AST, due to the behavior you are getting, and given your description of your visitors as "transforming" the code, which RecursiveASTVisitors don’t do by default — they just visit it, not modifying anything.  (Unless you're overriding TreeTransformer instead?)
>
> But you might also be getting this behavior if you are manually printing each Decl you want, in a RecursiveASTVisitor.  If this is the case, remember, struct { int i } s; is two distinct Decls, and that loop in DeclPrinter::VisitDeclContext that I linked to is necessary to get the proper printing behavior for such cases.
>
> There’s not quite enough information to know exactly what you need to do, but let’s start with the advice that that if you are manually printing individual Decls (e.g. D->print(OS)), that won’t work for your case — you need to instead handle a DeclContext as a whole and perhaps just copy and paste the important parts of DeclPrinter::VisitDeclContext and other parts of DeclPrinter.  If you’re still confused share a few more details — are you using RecursiveASTVisitor, are you printing from it, do you just want to print an output file etc.
>
> Good luck,
>
> Dave
>
> > On Feb 5, 2021, at 2:18 PM, John Leidel <[hidden email]> wrote:
> >
> > David, thanks for the response.  Please excuse my ignorance, but I'm a
> > bit confused as to the order this needs to occur.  I'll try to
> > reiterate what you said as follows:
> >
> > 1. If a TagDecl that is !isFreeStanding() is found, create a new
> > RecordDecl to represent it and add it to the DeclContext
> > 2. Save a reference to the newly created RecordDecl
> > 3. The next FieldDecl whose TagDecl matches the new RecordDecl should
> > be found; at this point create a new ElaboratedType using that TagDecl
> > 4. Create a new FieldDecl with the new ElaboratedType as the type
> >
> > Is this the general process?
> >
> > On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:
> >>
> >> Hi John,
> >>
> >> IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):
> >>
> >> https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410
> >>
> >> You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.
> >>
> >> To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)
> >>
> >> Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.
> >>
> >> Hope that works — good luck,
> >>
> >> Dave
> >>
> >> On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:
> >>
> >> I'm in the process of debugging (again) issues with anonymous struct
> >> decls.  I have the following basic structures located in a C source
> >> file that I'm attempting to transform.  I want to preserve the layout
> >> of the structs.
> >>
> >> struct{
> >> int A;
> >> }S;
> >>
> >> struct{
> >> int B;
> >> struct{
> >>   int C;
> >> }T;
> >> }R;
> >>
> >> When I use my AST visitors, I can pick up the RecordDecl for the
> >> struct definitions and the VarDecl for the variable definitions, but
> >> the transformed code is outputted as:
> >> struct{
> >> int A;
> >> };
> >> struct (anonymous) S;
> >>
> >> I can view the RecordDecl as as TagDecl and save off a reference to it
> >> if it is !isFreeStanding(), but how does one go about recombining the
> >> declaration of "S" and the original TagDecl?
> >>
> >> For the VarDecl ("S" in this case), I can determine whether or not its
> >> an "ElaboratedType", but I'm not sure how to utilize the saved
> >> reference to the RecordDecl:
> >>
> >> if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
> >>  /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
> >> }
> >> _______________________________________________
> >> cfe-dev mailing list
> >> [hidden email]
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >>
> >>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
John I think you’ve identified a bug: ElaboratedTypes are not being rebuilt with any OwnedTagDecl in TreeTransform.  

The last arg to getElaboratedType is the OwnedTagDecl which defaults to nullptr — and that default is being passed here:


Perhaps you can try fixing that so RebuildElaboratedType requires an NewOwnedTagDecl arg, and pass it the transformed version of the old OwnedTagDecl, and see if that gets it to print properly.

- Dave


On Feb 5, 2021, at 5:04 PM, John Leidel <[hidden email]> wrote:

David, thanks again for the response.  We are indeed transforming the
AST using combinations of new Decl's via Create* and tree transforms.
This is code we're helping to maintain (not the original authors), so
its a bit disjunct.

The original code is here if you're interested:
https://github.com/Intrepid/upc2c/blob/master/Transform.cpp#L2064

Basically, in the FieldDecl iterator loop, the RecordDecl's for the
struct definition are stored in the new DeclContext, but the
FieldDecl's are recreated.  I'm curious whether we check for an
ElaboratedType and if it exists for the target FieldDecl, utilize the
existing Decl for the new DeclContext.

Thoughts?

best
john


On Fri, Feb 5, 2021 at 3:23 PM David Rector <[hidden email]> wrote:

No that is not a general process — I assumed you had modified the AST by replacing the existing RecordDecl with a new one, and were printing the whole AST, due to the behavior you are getting, and given your description of your visitors as "transforming" the code, which RecursiveASTVisitors don’t do by default — they just visit it, not modifying anything.  (Unless you're overriding TreeTransformer instead?)

But you might also be getting this behavior if you are manually printing each Decl you want, in a RecursiveASTVisitor.  If this is the case, remember, struct { int i } s; is two distinct Decls, and that loop in DeclPrinter::VisitDeclContext that I linked to is necessary to get the proper printing behavior for such cases.

There’s not quite enough information to know exactly what you need to do, but let’s start with the advice that that if you are manually printing individual Decls (e.g. D->print(OS)), that won’t work for your case — you need to instead handle a DeclContext as a whole and perhaps just copy and paste the important parts of DeclPrinter::VisitDeclContext and other parts of DeclPrinter.  If you’re still confused share a few more details — are you using RecursiveASTVisitor, are you printing from it, do you just want to print an output file etc.

Good luck,

Dave

On Feb 5, 2021, at 2:18 PM, John Leidel <[hidden email]> wrote:

David, thanks for the response.  Please excuse my ignorance, but I'm a
bit confused as to the order this needs to occur.  I'll try to
reiterate what you said as follows:

1. If a TagDecl that is !isFreeStanding() is found, create a new
RecordDecl to represent it and add it to the DeclContext
2. Save a reference to the newly created RecordDecl
3. The next FieldDecl whose TagDecl matches the new RecordDecl should
be found; at this point create a new ElaboratedType using that TagDecl
4. Create a new FieldDecl with the new ElaboratedType as the type

Is this the general process?

On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:

Hi John,

IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):

https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410

You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.

To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)

Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.

Hope that works — good luck,

Dave

On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:

I'm in the process of debugging (again) issues with anonymous struct
decls.  I have the following basic structures located in a C source
file that I'm attempting to transform.  I want to preserve the layout
of the structs.

struct{
int A;
}S;

struct{
int B;
struct{
 int C;
}T;
}R;

When I use my AST visitors, I can pick up the RecordDecl for the
struct definitions and the VarDecl for the variable definitions, but
the transformed code is outputted as:
struct{
int A;
};
struct (anonymous) S;

I can view the RecordDecl as as TagDecl and save off a reference to it
if it is !isFreeStanding(), but how does one go about recombining the
declaration of "S" and the original TagDecl?

For the VarDecl ("S" in this case), I can determine whether or not its
an "ElaboratedType", but I'm not sure how to utilize the saved
reference to the RecordDecl:

if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
/// how do i recombine the VarDecl and the RecordDecl/TagDecl?
}
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev





_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
David, for reference, the code we're porting (referenced above) is
being merged up to Clang 9.0.1 (pre mono-repo).  The original
transform functions worked in Clang 3.9.0, so this may be a
regression.

That being said, I'm not sure I follow your potential solution.
Apologies for the naive response.  I don't usually work in the AST.

best
john

On Fri, Feb 5, 2021 at 5:26 PM David Rector <[hidden email]> wrote:

>
> John I think you’ve identified a bug: ElaboratedTypes are not being rebuilt with any OwnedTagDecl in TreeTransform.
>
> The last arg to getElaboratedType is the OwnedTagDecl which defaults to nullptr — and that default is being passed here:
>
> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L983
>
> Perhaps you can try fixing that so RebuildElaboratedType requires an NewOwnedTagDecl arg, and pass it the transformed version of the old OwnedTagDecl, and see if that gets it to print properly.
>
> - Dave
>
>
> On Feb 5, 2021, at 5:04 PM, John Leidel <[hidden email]> wrote:
>
> David, thanks again for the response.  We are indeed transforming the
> AST using combinations of new Decl's via Create* and tree transforms.
> This is code we're helping to maintain (not the original authors), so
> its a bit disjunct.
>
> The original code is here if you're interested:
> https://github.com/Intrepid/upc2c/blob/master/Transform.cpp#L2064
>
> Basically, in the FieldDecl iterator loop, the RecordDecl's for the
> struct definition are stored in the new DeclContext, but the
> FieldDecl's are recreated.  I'm curious whether we check for an
> ElaboratedType and if it exists for the target FieldDecl, utilize the
> existing Decl for the new DeclContext.
>
> Thoughts?
>
> best
> john
>
>
> On Fri, Feb 5, 2021 at 3:23 PM David Rector <[hidden email]> wrote:
>
>
> No that is not a general process — I assumed you had modified the AST by replacing the existing RecordDecl with a new one, and were printing the whole AST, due to the behavior you are getting, and given your description of your visitors as "transforming" the code, which RecursiveASTVisitors don’t do by default — they just visit it, not modifying anything.  (Unless you're overriding TreeTransformer instead?)
>
> But you might also be getting this behavior if you are manually printing each Decl you want, in a RecursiveASTVisitor.  If this is the case, remember, struct { int i } s; is two distinct Decls, and that loop in DeclPrinter::VisitDeclContext that I linked to is necessary to get the proper printing behavior for such cases.
>
> There’s not quite enough information to know exactly what you need to do, but let’s start with the advice that that if you are manually printing individual Decls (e.g. D->print(OS)), that won’t work for your case — you need to instead handle a DeclContext as a whole and perhaps just copy and paste the important parts of DeclPrinter::VisitDeclContext and other parts of DeclPrinter.  If you’re still confused share a few more details — are you using RecursiveASTVisitor, are you printing from it, do you just want to print an output file etc.
>
> Good luck,
>
> Dave
>
> On Feb 5, 2021, at 2:18 PM, John Leidel <[hidden email]> wrote:
>
> David, thanks for the response.  Please excuse my ignorance, but I'm a
> bit confused as to the order this needs to occur.  I'll try to
> reiterate what you said as follows:
>
> 1. If a TagDecl that is !isFreeStanding() is found, create a new
> RecordDecl to represent it and add it to the DeclContext
> 2. Save a reference to the newly created RecordDecl
> 3. The next FieldDecl whose TagDecl matches the new RecordDecl should
> be found; at this point create a new ElaboratedType using that TagDecl
> 4. Create a new FieldDecl with the new ElaboratedType as the type
>
> Is this the general process?
>
> On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:
>
>
> Hi John,
>
> IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):
>
> https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410
>
> You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.
>
> To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)
>
> Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.
>
> Hope that works — good luck,
>
> Dave
>
> On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:
>
> I'm in the process of debugging (again) issues with anonymous struct
> decls.  I have the following basic structures located in a C source
> file that I'm attempting to transform.  I want to preserve the layout
> of the structs.
>
> struct{
> int A;
> }S;
>
> struct{
> int B;
> struct{
>  int C;
> }T;
> }R;
>
> When I use my AST visitors, I can pick up the RecordDecl for the
> struct definitions and the VarDecl for the variable definitions, but
> the transformed code is outputted as:
> struct{
> int A;
> };
> struct (anonymous) S;
>
> I can view the RecordDecl as as TagDecl and save off a reference to it
> if it is !isFreeStanding(), but how does one go about recombining the
> declaration of "S" and the original TagDecl?
>
> For the VarDecl ("S" in this case), I can determine whether or not its
> an "ElaboratedType", but I'm not sure how to utilize the saved
> reference to the RecordDecl:
>
> if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
> /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
> }
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev


1. Go to your local lib/Sema/TreeTransform.h
2. Add an `OwnedTagDecl` param to `RebuildElaboratedType`; i.e. https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L979 is changed to:

```
QualType RebuildElaboratedType(SourceLocation KeywordLoc,
                               ElaboratedTypeKeyword Keyword,
                               NestedNameSpecifierLoc QualifierLoc,
                               QualType Named,
                               TagDecl *OwnedTagDecl) {
  return SemaRef.Context.getElaboratedType(Keyword,
                                           QualifierLoc.getNestedNameSpecifier(),
                                           Named,
                                           OwnedTagDecl);
}
```

3. Pass the proper arg in TransformElaboratedType, i.e. change https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L6197 to

```
TagDecl *NewOwnedTagDecl = T->getOwnedTagDecl() ? TransformDecl(SourceLocation(), T->getOwnedTagDecl()) : nullptr;
Result = getDerived().RebuildElaboratedType(TL.getElaboratedKeywordLoc(),
                                            T->getKeyword(),
                                            QualifierLoc, NamedT,
                                            NewOwnedTagDecl);
```

See if that works.


On Feb 5, 2021, at 6:48 PM, John Leidel <[hidden email]> wrote:

David, for reference, the code we're porting (referenced above) is
being merged up to Clang 9.0.1 (pre mono-repo).  The original
transform functions worked in Clang 3.9.0, so this may be a
regression.

That being said, I'm not sure I follow your potential solution.
Apologies for the naive response.  I don't usually work in the AST.

best
john

On Fri, Feb 5, 2021 at 5:26 PM David Rector <[hidden email]> wrote:

John I think you’ve identified a bug: ElaboratedTypes are not being rebuilt with any OwnedTagDecl in TreeTransform.

The last arg to getElaboratedType is the OwnedTagDecl which defaults to nullptr — and that default is being passed here:

https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L983

Perhaps you can try fixing that so RebuildElaboratedType requires an NewOwnedTagDecl arg, and pass it the transformed version of the old OwnedTagDecl, and see if that gets it to print properly.

- Dave


On Feb 5, 2021, at 5:04 PM, John Leidel <[hidden email]> wrote:

David, thanks again for the response.  We are indeed transforming the
AST using combinations of new Decl's via Create* and tree transforms.
This is code we're helping to maintain (not the original authors), so
its a bit disjunct.

The original code is here if you're interested:
https://github.com/Intrepid/upc2c/blob/master/Transform.cpp#L2064

Basically, in the FieldDecl iterator loop, the RecordDecl's for the
struct definition are stored in the new DeclContext, but the
FieldDecl's are recreated.  I'm curious whether we check for an
ElaboratedType and if it exists for the target FieldDecl, utilize the
existing Decl for the new DeclContext.

Thoughts?

best
john


On Fri, Feb 5, 2021 at 3:23 PM David Rector <[hidden email]> wrote:


No that is not a general process — I assumed you had modified the AST by replacing the existing RecordDecl with a new one, and were printing the whole AST, due to the behavior you are getting, and given your description of your visitors as "transforming" the code, which RecursiveASTVisitors don’t do by default — they just visit it, not modifying anything.  (Unless you're overriding TreeTransformer instead?)

But you might also be getting this behavior if you are manually printing each Decl you want, in a RecursiveASTVisitor.  If this is the case, remember, struct { int i } s; is two distinct Decls, and that loop in DeclPrinter::VisitDeclContext that I linked to is necessary to get the proper printing behavior for such cases.

There’s not quite enough information to know exactly what you need to do, but let’s start with the advice that that if you are manually printing individual Decls (e.g. D->print(OS)), that won’t work for your case — you need to instead handle a DeclContext as a whole and perhaps just copy and paste the important parts of DeclPrinter::VisitDeclContext and other parts of DeclPrinter.  If you’re still confused share a few more details — are you using RecursiveASTVisitor, are you printing from it, do you just want to print an output file etc.

Good luck,

Dave

On Feb 5, 2021, at 2:18 PM, John Leidel <[hidden email]> wrote:

David, thanks for the response.  Please excuse my ignorance, but I'm a
bit confused as to the order this needs to occur.  I'll try to
reiterate what you said as follows:

1. If a TagDecl that is !isFreeStanding() is found, create a new
RecordDecl to represent it and add it to the DeclContext
2. Save a reference to the newly created RecordDecl
3. The next FieldDecl whose TagDecl matches the new RecordDecl should
be found; at this point create a new ElaboratedType using that TagDecl
4. Create a new FieldDecl with the new ElaboratedType as the type

Is this the general process?

On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:


Hi John,

IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):

https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410

You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.

To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)

Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.

Hope that works — good luck,

Dave

On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:

I'm in the process of debugging (again) issues with anonymous struct
decls.  I have the following basic structures located in a C source
file that I'm attempting to transform.  I want to preserve the layout
of the structs.

struct{
int A;
}S;

struct{
int B;
struct{
int C;
}T;
}R;

When I use my AST visitors, I can pick up the RecordDecl for the
struct definitions and the VarDecl for the variable definitions, but
the transformed code is outputted as:
struct{
int A;
};
struct (anonymous) S;

I can view the RecordDecl as as TagDecl and save off a reference to it
if it is !isFreeStanding(), but how does one go about recombining the
declaration of "S" and the original TagDecl?

For the VarDecl ("S" in this case), I can determine whether or not its
an "ElaboratedType", but I'm not sure how to utilize the saved
reference to the RecordDecl:

if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
/// how do i recombine the VarDecl and the RecordDecl/TagDecl?
}
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev






_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Recombining AST Nodes

Deep Majumder via cfe-dev
David, thanks again for helping debug this.  Your fix required some
additional work to the template definitions for RebuildElaboratedType,
but this was otherwise successful!  If anyone is interested in the
patch, we have a single commit on our (modified) Clang 9.0.1 repo
here: https://github.com/tactcomplabs/clang-upc/commit/d30a2cc74387bc5bc613275368d221e3fbdbddce

The patch is simple enough that it can easily be merged into vanilla
Clang 9.0.1 repos.

David, I sincerely appreciate your ongoing help in debugging this.

best
john

On Fri, Feb 5, 2021 at 6:18 PM David Rector <[hidden email]> wrote:

>
>
>
> 1. Go to your local lib/Sema/TreeTransform.h
> 2. Add an `OwnedTagDecl` param to `RebuildElaboratedType`; i.e. https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L979 is changed to:
>
> ```
> QualType RebuildElaboratedType(SourceLocation KeywordLoc,
>                                ElaboratedTypeKeyword Keyword,
>                                NestedNameSpecifierLoc QualifierLoc,
>                                QualType Named,
>                                TagDecl *OwnedTagDecl) {
>   return SemaRef.Context.getElaboratedType(Keyword,
>                                            QualifierLoc.getNestedNameSpecifier(),
>                                            Named,
>                                            OwnedTagDecl);
> }
> ```
>
> 3. Pass the proper arg in TransformElaboratedType, i.e. change https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L6197 to
>
> ```
> TagDecl *NewOwnedTagDecl = T->getOwnedTagDecl() ? TransformDecl(SourceLocation(), T->getOwnedTagDecl()) : nullptr;
> Result = getDerived().RebuildElaboratedType(TL.getElaboratedKeywordLoc(),
>                                             T->getKeyword(),
>                                             QualifierLoc, NamedT,
>                                             NewOwnedTagDecl);
> ```
>
> See if that works.
>
>
> On Feb 5, 2021, at 6:48 PM, John Leidel <[hidden email]> wrote:
>
> David, for reference, the code we're porting (referenced above) is
> being merged up to Clang 9.0.1 (pre mono-repo).  The original
> transform functions worked in Clang 3.9.0, so this may be a
> regression.
>
> That being said, I'm not sure I follow your potential solution.
> Apologies for the naive response.  I don't usually work in the AST.
>
> best
> john
>
> On Fri, Feb 5, 2021 at 5:26 PM David Rector <[hidden email]> wrote:
>
>
> John I think you’ve identified a bug: ElaboratedTypes are not being rebuilt with any OwnedTagDecl in TreeTransform.
>
> The last arg to getElaboratedType is the OwnedTagDecl which defaults to nullptr — and that default is being passed here:
>
> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/TreeTransform.h#L983
>
> Perhaps you can try fixing that so RebuildElaboratedType requires an NewOwnedTagDecl arg, and pass it the transformed version of the old OwnedTagDecl, and see if that gets it to print properly.
>
> - Dave
>
>
> On Feb 5, 2021, at 5:04 PM, John Leidel <[hidden email]> wrote:
>
> David, thanks again for the response.  We are indeed transforming the
> AST using combinations of new Decl's via Create* and tree transforms.
> This is code we're helping to maintain (not the original authors), so
> its a bit disjunct.
>
> The original code is here if you're interested:
> https://github.com/Intrepid/upc2c/blob/master/Transform.cpp#L2064
>
> Basically, in the FieldDecl iterator loop, the RecordDecl's for the
> struct definition are stored in the new DeclContext, but the
> FieldDecl's are recreated.  I'm curious whether we check for an
> ElaboratedType and if it exists for the target FieldDecl, utilize the
> existing Decl for the new DeclContext.
>
> Thoughts?
>
> best
> john
>
>
> On Fri, Feb 5, 2021 at 3:23 PM David Rector <[hidden email]> wrote:
>
>
> No that is not a general process — I assumed you had modified the AST by replacing the existing RecordDecl with a new one, and were printing the whole AST, due to the behavior you are getting, and given your description of your visitors as "transforming" the code, which RecursiveASTVisitors don’t do by default — they just visit it, not modifying anything.  (Unless you're overriding TreeTransformer instead?)
>
> But you might also be getting this behavior if you are manually printing each Decl you want, in a RecursiveASTVisitor.  If this is the case, remember, struct { int i } s; is two distinct Decls, and that loop in DeclPrinter::VisitDeclContext that I linked to is necessary to get the proper printing behavior for such cases.
>
> There’s not quite enough information to know exactly what you need to do, but let’s start with the advice that that if you are manually printing individual Decls (e.g. D->print(OS)), that won’t work for your case — you need to instead handle a DeclContext as a whole and perhaps just copy and paste the important parts of DeclPrinter::VisitDeclContext and other parts of DeclPrinter.  If you’re still confused share a few more details — are you using RecursiveASTVisitor, are you printing from it, do you just want to print an output file etc.
>
> Good luck,
>
> Dave
>
> On Feb 5, 2021, at 2:18 PM, John Leidel <[hidden email]> wrote:
>
> David, thanks for the response.  Please excuse my ignorance, but I'm a
> bit confused as to the order this needs to occur.  I'll try to
> reiterate what you said as follows:
>
> 1. If a TagDecl that is !isFreeStanding() is found, create a new
> RecordDecl to represent it and add it to the DeclContext
> 2. Save a reference to the newly created RecordDecl
> 3. The next FieldDecl whose TagDecl matches the new RecordDecl should
> be found; at this point create a new ElaboratedType using that TagDecl
> 4. Create a new FieldDecl with the new ElaboratedType as the type
>
> Is this the general process?
>
> On Fri, Feb 5, 2021 at 9:57 AM David Rector <[hidden email]> wrote:
>
>
> Hi John,
>
> IIUC you are creating a new RecordDecl, manually replacing the old one with the new one in its DeclContext, and then printing it.  As you seem to recognize this is the problematic line (recall this is from that DeclPrinter code I referenced the last time):
>
> https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclPrinter.cpp#L410
>
> You need the FieldDecl (which should be the very next Decl* in the DeclContext after the old RecordDecl *) to have an elaborated type whose owned tag decl is the new RecordDecl you have created (which will be `Decls[0]` in that code); if it is thus, and if the RecordDecl you created is marked as !isFreeStanding(), it should print as you expect, I think.
>
> To create that type, use the ASTContext::getElaboratedType(…) method, which accepts the OwnedTagDecl as an arg.  (ASTContext manages the types to ensure there are not multiple copies of the same Type* floating around, IIUC.)
>
> Then, I would probably also replace the existing FieldDecl with a newly Created one, in the same manner you replaced the RecordDecl *, only now Create it with the new type.
>
> Hope that works — good luck,
>
> Dave
>
> On Feb 4, 2021, at 9:59 AM, John Leidel via cfe-dev <[hidden email]> wrote:
>
> I'm in the process of debugging (again) issues with anonymous struct
> decls.  I have the following basic structures located in a C source
> file that I'm attempting to transform.  I want to preserve the layout
> of the structs.
>
> struct{
> int A;
> }S;
>
> struct{
> int B;
> struct{
> int C;
> }T;
> }R;
>
> When I use my AST visitors, I can pick up the RecordDecl for the
> struct definitions and the VarDecl for the variable definitions, but
> the transformed code is outputted as:
> struct{
> int A;
> };
> struct (anonymous) S;
>
> I can view the RecordDecl as as TagDecl and save off a reference to it
> if it is !isFreeStanding(), but how does one go about recombining the
> declaration of "S" and the original TagDecl?
>
> For the VarDecl ("S" in this case), I can determine whether or not its
> an "ElaboratedType", but I'm not sure how to utilize the saved
> reference to the RecordDecl:
>
> if(isa<ElaboratedType>(SemaRef.Context.getBaseElementType(MyVarDecl))) {
> /// how do i recombine the VarDecl and the RecordDecl/TagDecl?
> }
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev