Member expression returns type with wrong qualifiers

As I mentioned before, the following:
int a(const struct {int a;}*b) {b->a = 10;}
is incorrectly accepted by clang. (Relevant spec reference: C99 6.5.2.3p3.)

I'm not completely sure how to go about fixing this, though. It could
be fixed completely in the MemberExpr constructor, but that would be
burying spec rules inside Expr.h, which seems wrong. (Not to mention
it would be extremely ugly.) On the other hand, if the logic isn't in
the constructor, serialization also needs to be able to figure out the
type of the MemberExpr. Maybe some sort of helper would be best?

-Eli

One solution is to extend Expr::isModifiableLvalue() and the MemberExpr AST to return the underlying base type.

I've enclose a quick prototype that seems to solve the problem (for discussion purposes...).

snaroff

[snaroff:llvm/tools/clang] snarofflocal% svn diff
Index: include/clang/AST/Expr.h

That might fix this exact case, but it's not a general fix. Try
something like the following:
int a(const struct {int a;} * x) {
int* y = &x->a;
*y = 10;
}

(Note that we actually allow this for gcc compat, but the -pedantic
warning that should show up doesn't show up.)

Or the following:
int a(volatile struct {int a;} * x) {
x->a = 10;
}

Which will silently generate wrong code because it throws out the
volatile qualifier.

Pretty much anything that depends on qualifiers would require an
independent fix for this issue, which seems like a bad idea.

-Eli

One solution is to extend Expr::isModifiableLvalue() and the
MemberExpr AST to return the underlying base type.

That might fix this exact case, but it's not a general fix. Try
something like the following:
int a(const struct {int a;} * x) {
int* y = &x->a;
*y = 10;
}

O.K. Here's a crazy idea...have you considered turning...

const struct whatever { int a; };

...into...

const struct whatever { const int a; };

In other words, pass down the const-ness to ActOnField()? Then the qualifiers could be added to all the FieldDecls...

The only downside is distinguishing between "implicit/inherited" qualifiers from qualifiers that are explicit in the source (but that can obviously be fixed by saving the original type, if necessary).

This declaration-centric solution has some appeal...MemberExpr/Expr wouldn't need to change.

snaroff

Possible, I suppose, but it seems complicated to deal with stuff like
type compatibility. Also, probably a bigger issue, it would make
getUnqualifiedType more expensive. This would also be throwing out
the other advantages of QualType's for structs. I'll have to think
about it a bit more, though.

Below is something closer to my original line of thought; it's a lot
shorter than anything involving messing with types. It's not
especially clean, but it works for all cases. (Some extra code will
be required to deal with address spaces, though.)

The other possibility I was considering was making MemberExpr
serialize their type, and then moving this logic into sema.

-Eli

Index: include/clang/AST/Expr.h

O.K. Here's a crazy idea...have you considered turning...

const struct whatever { int a; };

...into...

const struct whatever { const int a; };

In other words, pass down the const-ness to ActOnField()? Then the
qualifiers could be added to all the FieldDecls...

The only downside is distinguishing between "implicit/inherited"
qualifiers from qualifiers that are explicit in the source (but that
can obviously be fixed by saving the original type, if necessary).

This declaration-centric solution has some appeal...MemberExpr/Expr
wouldn't need to change.

snaroff

Possible, I suppose, but it seems complicated to deal with stuff like
type compatibility. Also, probably a bigger issue, it would make
getUnqualifiedType more expensive. This would also be throwing out
the other advantages of QualType's for structs. I'll have to think
about it a bit more, though.

Below is something closer to my original line of thought; it's a lot
shorter than anything involving messing with types. It's not
especially clean, but it works for all cases. (Some extra code will
be required to deal with address spaces, though.)

The other possibility I was considering was making MemberExpr
serialize their type, and then moving this logic into sema.

The code below looks good.

I think it would be a bit cleaner to fold the getTypeForMemberExpr() logic into Sema::ActOnMemberReferenceExpr() and pass in the derived type explicitly (just add an argument to the constructor). Passing the type in explicitly is more consistent with all the other expr AST's.

snaroff

So, more like the attached?

-Eli

memberexprfix.txt (3.5 KB)

I think it would be a bit cleaner to fold the getTypeForMemberExpr()
logic into Sema::ActOnMemberReferenceExpr() and pass in the derived
type explicitly (just add an argument to the constructor). Passing the
type in explicitly is more consistent with all the other expr AST's.

So, more like the attached?

Exactly. Looks great.

Please add a test for this bug/patch and commit.

Thanks,

snaroff

Steve Naroff wrote:-

>> I think it would be a bit cleaner to fold the getTypeForMemberExpr()
>> logic into Sema::ActOnMemberReferenceExpr() and pass in the derived
>> type explicitly (just add an argument to the constructor). Passing
>> the
>> type in explicitly is more consistent with all the other expr AST's.
>
> So, more like the attached?
>

Exactly. Looks great.

Please add a test for this bug/patch and commit.

Thanks,

Now we have qualifers elsewhere, don't we need to do more
than or-ing qualifiers?

Neil.

Address-space qualifiers are highly experimental in both clang and
llvm; I don't think we need to worry about them yet.

-Eli

That said, a note or FIXME would be appreciated since it’s known that work will be needed in this spot.

N.B. I’d say that address spaces in LLVM are a lot less experimental than their support in clang.