Bad code on shared assignment of anon struct

[PHHargrove]

Given this 2-line UPC code:

struct A { struct { int c; int d; } b; };
void baz(shared struct A *p, struct A *q) { p->b = q->b; }

The translation of baz() looks like:

void baz(upcr_pshared_ptr_t p, struct A *q) {
    UPCR_BEGIN_FUNCTION();
    struct <anonymous struct at newbug.upc:1:12> _bupc_spilld0;
    {   
        (_bupc_spilld0 = q->b , upcr_put_pshared(p, 0UL, &_bupc_spilld0, 8UL) , _bupc_spilld0);
    }
    UPCR_EXIT_FUNCTION();
}

The problem is the type for the spill: struct <anonymous struct at newbug.upc:1:12>

Two observations:

1. I think there is existing code to create the anon struct defns as needed, but couldn't figure out how to apply it to this case.
2. Unrelated to the bug itself is the observation that there is no need for a spill here, since q->b is a lvalue and therefore &(q->b) would be legal in the communication call.

I will be adding this test code to Berkeley test suite shortly.

[PHHargrove]

The following 1-line change almost worked:

--- a/Transform.cpp
+++ b/Transform.cpp
@@ -1591,6 +1591,7 @@ namespace {
        }
       } else if(RecordDecl *RD = dyn_cast<RecordDecl>(D)) {
        IdentifierInfo *Name = getRecordDeclName(RD->getIdentifier());
+       if(!Name) Name = &SemaRef.Context.Idents.get((Twine("_bupc_anon_tag") + Twine(AnonRecordID++)).str());
        RecordDecl *Result = RecordDecl::Create(SemaRef.Context, RD->getTagKind(), DC,
                                  RD->getLocStart(), RD->getLocation(),
                                  Name, cast_or_null<RecordDecl>(TransformDecl(SourceLocation(), RD->getPreviousDecl())));

It adds tags to any unnames structs or unions within any transformed decl.
So, it was sufficient to fix the problem I reported.

However, I then realized that it cannot help for structures that might appear in system headers.
Additionally, I am not too fond of the idea of modifying all structure or union defns "just in case" we need to create a spill of that type.
So, I have stared work on an "on demand" solution which I will push for review momentarily.

[swatanabe]

AMDG

On 03/20/2014 01:26 AM, Paul H. Hargrove wrote:

However, I then realized that it cannot help for structures that might appear in system headers.
Additionally, I am not too fond of the idea of modifying all structure or union defns "just in case" we need to create a spill of that type.
So, I have stared work on an "on demand" solution which I will push for review momentarily.

1. I don't think that checking if the type is a
   RecordDecl is sufficient. A pointer type can
   contain an anonymous record type:

struct A { struct { int x; } *ptr };

2. Please don't copy the code for transforming a RecordDecl.
   It should be factored into a separate function.

3. The fact that there are two separate types will
   cause compiler errors in the assignment. We need
   to insert casts, which of course is a problem when
   we don't have a way to refer to the type...
   When we have an lvalue of the original type, we
   can cast the address to a pointer to the new type,
   but I have no idea how do deal with rvalues.

4. getAs strips off Typedefs. If we have a TypedefType,
   there's no need to create an new declaration.

In Christ,
Steven Watanabe

[swatanabe]

I /think/ that this problem can only happen for anonymous
structs that are nested in another struct. (If the anonymous
struct is at the top level and the declaration is shared,
then we need to fix it up anyway. If it isn't shared, then
we don't need to transform anything, and there's no problem.)
So, what if we created a variable of the outer struct type instead?

[PHHargrove]

Steven,

Thanks for the review.
Regarding the initial comments:

1. Good point. If we keep the code then there is probably more checking needed before activating this path. For the case you show I would still need a way to express typeof(ptr) to create a spill for A.ptr=B.ptr, but use of void* is an option for pointers that we don't have for other members.

2. This code does not transform, since Ty is for a type already transformed earlier. So, the code is only cloning the type, and therefore differs from the "other" instance by not calling TransformType(), TransformExpr() or transformedLocalDecl(). IFF you tell me it is safe to transform a second time then I think I could safely use a piece of factored code.

3. I did not get compile errors in my testing, but I've only tried with gcc-4.7.0 as the backend. The resulting new type exactly fits the definition of a compatible type (struct and union cases) as defined in ISO C99 section 6.2.7 except for

If one is declared with a tag, the other shall be declared with the same tag

4. OK, point taken. So, then how does one query "what is a typedef name for this QualType?"

As you observe in your second comment we are (I agree) only dealing with a struct or union contained within an outer one.

So, I suppose it is possible to create a spill with the outer type, though I have no clue how to discover what that type is, and fear building the A.b.c.d.e.f.g expression needed to access its appropriate member.

Please note that this problem with assignment was discovered while working on issue #78 by trying to emit calls to sizeof() and offsetof() in lieu of integer literals (in Load, Store and MemberExpr for now, but maybe everywhere eventually). With the use of a spill of the outer type, I believe one can write the sizeof() expression because one can use the field name: sizeof(spill.b). However, offsetof() needs a type name and I don't see a way to get one other than the path I've taken with the current attempt.

At this point I feel that I am reaching beyond my abilities in this code base. I am willing to continue work if given some guidance. I can address #1, and will address #2 if somebody reassures me that multiple-transformation is not a problem. However, I simply don't know enough about how Clang's AST Classes work to address any of the other issues.

FWIW: I don't see this bug as representing a common usage.

[swatanabe]

AMDG

On 03/20/2014 10:46 AM, Paul H. Hargrove wrote:

1. Good point. If we keep the code then there is probably more checking needed before activating this path. For the case you show I would still need a way to express typeof(ptr) to create a spill for A.ptr=B.ptr, but use of void* is an option for pointers that we don't have for other members.

Is casting to void * guaranteed to be safe?
I know in practice all pointers are interchangeable,
but I'm not sure that the standard guarantees it.

2. This code does not transform, since Ty is for a type already transformed earlier. So, the code is only cloning the type, and therefore differs from the "other" instance by not calling TransformType(), TransformExpr() or transformedLocalDecl(). IFF you tell me it is safe to transform a second time then I think I could safely use a piece of factored code.

Ah, right. TransformExpr and TransformType should
be safe, but transformedLocalDecl could be a problem.
It can probably be made to work, but it's not worth
the effort.

3. I did not get compile errors in my testing, but I've only tried with gcc-4.7.0 as the backend. The resulting new type exactly fits the definition of a compatible type (struct and union cases) as defined in ISO C99 section 6.2.7 except for

If one is declared with a tag, the other shall be declared with the same tag

4. OK, point taken. So, then how does one query "what is a typedef name for this QualType?"

Checking getAs() != NULL should be enough.
If we wanted to know whether /any/ typedef exists, we'd
need to build a lookup table.

As you observe in your second comment we are (I agree) only dealing with a struct or union contained within an outer one.

So, I suppose it is possible to create a spill with the outer type, though I have no clue how to discover what that type is, and fear building the A.b.c.d.e.f.g expression needed to access its appropriate member.

We'd need to create an auxiliary table that maps
from a nested struct to the containing struct.
Then we can walk up until we find a struct
with a name.

Please note that this problem with assignment was discovered while working on issue #78 by trying to emit calls to sizeof() and offsetof() in lieu of integer literals (in Load, Store and MemberExpr for now, but maybe everywhere eventually). With the use of a spill of the outer type, I believe one can write the sizeof() expression because one can use the field name: sizeof(spill.b). However, offsetof() needs a type name and I don't see a way to get one other than the path I've taken with the current attempt.

Agreed.

At this point I feel that I am reaching beyond my abilities in this code base. I am willing to continue work if given some guidance. I can address #1, and will address #2 if somebody reassures me that multiple-transformation is not a problem. However, I simply don't know enough about how Clang's AST Classes work to address any of the other issues.

I think the most important thing is to work out what
the possible inputs are, and what they should be
transformed into. Once we agree on that, I can implement
it fairly quickly.

FWIW: I don't see this bug as representing a common usage.

In Christ,
Steven Watanabe

[PHHargrove]

To respond to Steven's points in the previous comment (but with pruned context):

Is casting to void * guaranteed to be safe?
I know in practice all pointers are interchangeable,
but I'm not sure that the standard guarantees it.

My recollection is that the standard requires certain classes of pointers be interchangeable, but that there are at least two classes: objects and functions. Since we are talking only about anonymous record types, I am 99% certain that interchangeability w/ void* is required by the standard.

Ah, right. TransformExpr and TransformType should
be safe, but transformedLocalDecl could be a problem.
It can probably be made to work, but it's not worth
the effort.

In that case it would be simple to have a bool paramater to the factored function that controlled the hand full of calls to transformedLocalDecl(). Unlike the Transform*() calls they are on lines by themselves, not in inconvenient places like the parameter lists for other calls.

Checking getAs() != NULL should be enough.
If we wanted to know whether /any/ typedef exists, we'd
need to build a lookup table.

OK. I don't think we reach the current path for a typedef but will add the check if/when I get back to this code.
I actually thought you were asking me to pursue the "any typedef exists" case.

[To reconstruct A.b.c.d to reach a member inside a nested anon struct or union inside a named one]

We'd need to create an auxiliary table that maps
from a nested struct to the containing struct.
Then we can walk up until we find a struct
with a name.

That is pretty much what I feared.

I think the most important thing is to work out what
the possible inputs are, and what they should be
transformed into. Once we agree on that, I can implement
it fairly quickly.

I'd be relieved to have you take over on this issue.
Maybe we'll have time to discuss on the conf call (about 10 minutes away), and if not we can pursue by email rather than the issue tracker.

[PHHargrove]

Some specification:

The inputs:

• a QualType (or reference to one) already transformed and already stripped of qualifiers via getUnqualifiedType()

The result:

• IF the type has a name then no side-effects are expected
• IF the type is an anonymous (no tag) struct or union we need EITHER:
  • a new type that does have a name and is "interchangable" with the original
  • OR
  • to rewrite the existing type to add a tag to the struct or union

Expected usage cases for the new or rewritten type:

• sizeof(Type) for Get and Put calls in Load and Store
• offsetof(Type, member) for addr arithmetic in MemberExpr
• as the Type for variables generated by createTmpVar()
• eventually sizeof(Type) for all pointer-to-shared arithmetic (the elmsz argument)

The first two cases on that list (sizeof in Load/Store and offsetof in MemberExpr) were already prototyped via new createSizeOf() and createOffsetOf() functions (lines 640 to 659 in https://github.com/PHHargrove/upc2c/blob/issue-78-v2/Transform.cpp ). When implemented, the fourth case would use that same createSizeOf().

So, the new code might have exactly three callers: create{TmpVar,SizeOf,OffsetOf}()

[PHHargrove]

to rewrite the existing type to add a tag to the struct or union

It may be worth noting again that I tried a variant on that that idea (one-line patch in #83 (comment)) that unconditionally rewrote all records types as they were initially processed, by inserting tags on any that lacked one. The problem with that approach is lack of coverage for structs or unions defined in system headers. Even though clang's AST may now have a tag for the type, the backend compiler won't have it.

That still might turn out to be the best approach available, though it just makes the bug "smaller" rather than eliminating it.