On 10/09/2015 04:55 AM, Martin Sebor wrote:
Gcc attempts to diagnose invalid offsetof expressions whose member
designator is an array element with an out-of-bounds index. The
logic in the function that does this detection is incomplete, leading
to false negatives. Since the result of the expression in these cases
can be surprising, this patch tightens up the logic to diagnose more
such cases.
In the future, please explain more clearly in the patch submission what
the false negatives are. That'll make the reviewer's job easier.
Tested by boostrapping and running c/c++ tests on x86_64 with no
regressions.
Should run the full testsuite (standard practice, and library tests
might have occurrences of offsetof).
A ChangeLog is missing. (Not that I personally care about ChangeLogs,
but apparently others do.)
+struct offsetof_ctx_t
+{
+ tree inx; /* The invalid array index or NULL_TREE. */
+ int maxinx; /* All indices to the array have the highest valid value. */
+};
I think "idx" is commonly used, I've never seen the spelling "inx".
Also, typically comments go on their own lines before the field.
+
+ if (tree_int_cst_lt (upbound, t)) {
+ pctx->inx = t;
None-standard formatting. Elsewhere too.
+ /* Index is considered valid when it's either less than
+ the upper bound or equal to it and refers to the lowest
+ rank. Since in the latter case it may not at this point
+ in the recursive call to the function be known whether
+ the element at the index is used to do more than to
+ compute its offset (e.g., it can be used to designate
+ a member of the type to which the just past-the-end
+ element refers), point the INX variable at the index
+ and leave it up to the caller to decide whether or not
+ to diagnose it. Special handling is required for minor
+ index values referring to the element just past the end
+ of the array object. */
I admit to having trouble parsing this comment. Can you write that in a
clearer way somehow? I'm still trying to make my mind up whether the
logic in this patch could be simplified.
t = convert (sizetype, t);
off = size_binop (MULT_EXPR, TYPE_SIZE_UNIT (TREE_TYPE (expr)), t);
+
break;
Spurious change, please remove.
+extern tree fold_offsetof_1 (tree, offsetof_ctx_t* = NULL);
Space before *.
+// treatment since the offsetof exression yields the same result for
"expression".
+ // The following expression is silently accepted as an extension
+ // because it simply forms the equivalent of a just-past-the-end
+ // address.
+ __builtin_offsetof (A, a1_1 [0][1]), // extension
Hmm, do we really want to support any kind of multidimensional array for
this extension? My guess would have been to warn here.
So I checked and it looks like we accept flexible array member syntax
like "int a[][2];", which suggests that the test might have the right
idea, but has the indices swapped (the first one is the flexible one)?
Ccing Joseph for a ruling.
Bernd
