Type Qualifier modifiers problem

[counc009]

The way that the applyMods (and applyLhsRhsMods) functions work to apply RuntimeMods causes problems with lot of the productions they can operate on, because the function uses stmtExpr in wrapping the modified expressions. Because of this, any injectable production that expects an lValue (such as pre and post increment and decrement) will fail if we try to use qualifiers to modify the expression (since a stmtExpr must always be an rValue).
I think it is safe to remove this stmtExpr, the risk this introduces would be for the original expression to be evaluated multiple times, however using a stmtExpr means that anywhere that requires an lValue will be an error.
Looking at a longer term fix to this, it seems like the model used for handling qualifiers in CallExpr might work better in general, where it is possible to inject Stmt before and after the expression itself, transforming the expression into a stmtExpr that does setup, evaluates the expression, and then does more afterwards, finally returning the value from the function call. This model would work for any production that produces an rValue. The only productions I can think of that produce lValues are the dereferenceExpr and arraySubscriptExpr, though in both of these cases the children are rValues, and so a stmtExpr for each child can be used.
I don't know whether this is the correct fix, or what it is, but I thought it was something worth bringing up. I'm going to make a pull request for the initial idea (just removing the stmtExpr), but I think further discussion of how to better handle this may be needed.

[krame505]

Yeah, there is a lot that could be improved with the design of injection in ableC. I would chat with @TravisCarlson about this, he originally implemented the injection/runtime modification stuff.

[TravisCarlson]

The problem as I understand it is that injecting code that transforms an expression that is an lvalue into an expression that is not an lvalue breaks programmer-written code. For example, if the programmer writes:

int a;
a++;

this is fine at the host-language level. But it's not ok if we do something at the injectable level that transforms the expression a being incremented to an expression statement ({ int tmp = a; __a[0]; }) that is not an lvalue, like this:

int a;
({ int tmp = a; tmp; })++;

However, this problem is unique to the operators like ++ that require their operands to be lvalues. Because, although expression statements are not themselves lvalues, they can be contained in expressions that are lvalues.

Another example:

int a[] = {1, 2, 3, 4};
(a[0])++;

And suppose we inject on array subscripting to generate:

int a[] = {1, 2, 3, 4};
(({ int *tmp = a; tmp; })[0])++;

This is not broken. So I'd say it appears that the operators that require lvalues on their operands are different than the other operators, that injection on those operators only is broken in this way, and that a fix should probably touch only the broken injection code on those operators. Because as you mention, the statement expressions are useful in that they prevent the expression from being evaluated more than once, and I'd prefer not to remove that where we don't have to.

So the above is my response to the current problems with injection in ableC. Am I misunderstanding or not thinking of anything? Below is my response regarding the specific use case this is intended to be used for.

If I understand the specific use case, the programmer-written code is something like:

int __a[] = {1, 2, 3, 4};
int a;
a++;

and the desired generated code is:

int __a[] = {1, 2, 3, 4};
int a;
__a[0]++;

but the actual (erroneous) generated code is:

int __a[] = {1, 2, 3, 4};
int a;
({ int *tmp = a; __a[0]; })++;

There are three productions that construct the RuntimeMod nonterminal: runtimeCheck, runtimeConversion, and runtimeInsertion. Only runtimeConversion allows the final statement in the generated code to be something other than tmp; so I guess this is what's being used in this case. The use case that runtimeConversion was created for was to automatically convert dimensional units, for example to convert an expression length in units of meters to length * 1000 when millimeters are required.

In contrast to this dimensional example, it is possible to use runtimeConversion to generate a new value that makes no use of the original expression, for example to convert a++ into an expression that makes no use of a. But doing that just feels weird and like a lot could go wrong. I think it would be a good idea to think very carefully and look for alternative solutions before doing that.

I still don't fully understand the problem that this solution is intending to solve. Are we sure that this is the best (or only) way to solve it?

[krame505]

@TravisCarlson, I think that is more or less the situation.

@counc009, I do agree with Travis on this - are you sure there isn't a better approach? Have you even considered just defining a new attribute over all of ableC that translates a piece of code into one where the variables have been modified as required?

[counc009]

Yeah, that seems accurate. Yeah, the problem is really isolated to productions expecting lValues. A solution that is more direct would probably be better.

@krame505 Yeah, that might be a better implementation. To do this, is there a good list of all nonterminals who children may include Expr? Also, a question on the Silver side: Is it possible to use propagate in a default aspect production? It would be easier if possible, but I think I tried once and it didn't seem to work.

[krame505]

Unfortunately the answer to "which nonterminals have children that may include Expr" is "all of them", except for types and a few other random cases. But you might just choose not to support e.g. referencing an expression from inside a GCC extension type attribute or sizeof type, so I would suggest starting out with just Stmt, Expr, Decl, the list versions of these, and whatever else is something that is commonly used in a function or seems useful to support.

No, propagate only works in regular and aspect productions unfortunately, although being able to do it for an entire nonterminal is definitely something we want but nobody has had time to do. I hadn't thought of doing it as just being able to write propagate in an aspect default production, but that would be a pretty elegant approach. @ericvanwyk this might actually be something good for Louis to work on over the summer, if you are still looking for ideas. We also never got around to making propagate work for monoid attributes...

For now the quickest option is probably to go look at the history of ableC and find the substitution grammar from before last week when we switched everything over to use reflection - we had a whole bunch of aspect stubs for all of ableC that just have propagate substituted; as their body, you could just do a quick find-and-replace.