Page MenuHomePhabricator

warning for restrict-qualified pointer assignments with undefined behavior
Needs ReviewPublic

Authored by hfinkel on Oct 21 2014, 12:15 PM.

Details

Summary

This patch introduces a warning for undefined behavior introduced by assignments to a restrict-qualified pointer an expression based on other restrict qualified pointers. Roughly speaking, this happens when the LHS is designated by an identifier with a scope contained by the scope of the RHS designator (you can't assign from an "inner" restrict pointer to an "outer" one -- or between two at the same level).

For example, if we have:

struct S {
  int * restrict x;
  int * restrict y;
};

void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
{
  s.x = q1;
  t->y = s.x;
}

you'll get these warnings (with some hopefully-useful notes):

warning: assignment to a restrict-qualified pointer designated by 's' using an expression based on a restrict-qualified
      pointer designated by 'q1' has undefined behavior [-Wundefined-restrict-assignment]
  s.x = q1;
      ^
note: the block associated with 'q1', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                       ^
note: does not begin before the block associated with 's', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                                                       ^
warning: assignment to a restrict-qualified pointer designated by 't' using an expression based on a restrict-qualified
      pointer designated by 's' has undefined behavior [-Wundefined-restrict-assignment]
  t->y = s.x;
       ^
note: the block associated with 's', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                                                        ^
note: does not begin before the block associated with 't', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                                                                    ^

Currently, this only determines the "based on" relationship via a syntactic analysis of the expression. In the future, I'd like to make this an analysis-based warning so that some data-flow information can inform the "based on" determination in case it flows through other non-restrict-qualified pointers.

Thanks again!

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 15198.Oct 21 2014, 12:15 PM
hfinkel retitled this revision from to warning for restrict-qualified pointer assignments with undefined behavior.
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Nov 11 2014, 7:36 PM

The restriction on assigning between restrict-qualified pointers at the same level seems like a bug in the C spec to me. It makes no sense to me that

int *restrict p = /*...*/;
{
  int *restrict q = p;
}

should be fine, but removing the braces should make it UB.

lib/Sema/SemaExpr.cpp
8624

I don't think this is what you mean. Linkage is not relevant to the restrict rules as far as I can see; I think you're trying to check the storage class here. Maybe VarDecl::hasGlobalStorage() is what you want?

8626–8629

Needs more braces. =)

8634–8638

I would think we should walk up to the block of the member function for a C++ this expression. Calling a restrict-qualified member function on a non-restrict object should only apply the restriction for the scope of the function.

8640

I'd expect this assert to fire when defining a member function out of line.

8646–8647

You should only look at the chosen subexpression. Choose is supposed to be entirely transparent once its subexpression is chosen.

8651–8653

You can't assume the array operand is on the left-hand side. getBase?

8656–8659

I don't think this is right in a lot of cases. For instance, you walk from a CallExpr to its arguments. Since you're going to tell the user their code has undefined behavior, you should err towards false negatives.

8682–8683

Weird indentation (here and elsewhere). Tabs?

8811–8812

You're using scopes pretty heavily here, but this code can be reached from template instantiation, where there are no scopes (and because restrict is part of a type, it's possible for the restrictness to not be known until instantiation). Maybe the easiest thing to do for now is to just skip all of this if we're performing template instantiation.

This stuff also won't work as-written with dependent types and dependent expressions.

aaron.ballman resigned from this revision.Oct 13 2015, 5:55 AM
aaron.ballman removed a reviewer: aaron.ballman.