On 12/07/2016 11:43 AM, Jeff Law wrote:
On 12/02/2016 03:54 PM, Martin Sebor wrote:
Thanks for looking at this! I realize it's dense code and not easy
to make sense out of.
+
+ if (TREE_CODE (*argmin) == INTEGER_CST
+ && TREE_CODE (*argmax) == INTEGER_CST
+ && (dirprec >= argprec
+ || integer_zerop (int_const_binop (RSHIFT_EXPR,
+ int_const_binop (MINUS_EXPR,
+ *argmax,
+ *argmin),
+ size_int (dirprec)))))
+ {
+ *argmin = force_fit_type (dirtype, wi::to_widest (*argmin), 0,
false);
+ *argmax = force_fit_type (dirtype, wi::to_widest (*argmax), 0,
false);
+ return false;
+ }
So in this case we're not changing the values, we're just converting
them to a equal or wider type, right? Thus no adjustment (what about a
sign change? Is that an adjustment?) and we return false per the
function's specifications.
This casts the value to the destination type, so it may result in
different values. The important postcondition it preserves is that
the difference ARGMAX - ARGMIN is less than or equal to TYPE_MAX of
the directive's unsigned TYPE. (If it isn't, the range cannot be
converted.) This lets us take, say:
int f (int i)
{
if (i < 1024 || 1033 < i)
i = 1024;
return snprintf (0, 0, "%hhi", i);
}
and fold the function call into 1 because (signed char)1024 is 0 and
(signed char)1033 is 9, and every other value in that same original
range is also in the same range after the conversion. We know it's
safe because (1033 - 1024 <= UCHAR_MAX) holds.
But the code in question is guarded by dirprec >= argprec. Thus aren't
we converting to an equal or wider type? In the case of converting to a
wider type, ISTM the values won't change and thus we return false.
I just saw your other response after I typed this up (thanks!) but
so just to close the loop:
You're right, when converting to a wider type the values won't change.
The guard is an OR expression:
dirprec >= argprec
OR
0 == ((ARGMAX - ARGMIN) >> TYPE_PRECISION (dirtype))
and so when (dirprec < argprec) holds the values might change if
the other operand is true. In both of these cases the function
returns false to indicate that the original range the arguments
were in hasn't changed as a result.
If we are converting to the same sized type, but with a different
signedness, then the values will have been adjusted and ISTM we ought to
be returning true in that case. But the code actually returns false.
I must be missing something here.
The return value of the function is only used to decide the phrasing
of the notes printed after warnings and what values to include in
them. Returning false means that the notes will say something like
"directive argument in the range [ARGMIN, ARGMAX]" where the values
are those determined by VRP. Returning true means that the note
will instead say "using the range [TYPE_MIN, 0] for directive
argument." This distinction will (I hope) let the user know that
the second case is less accurate than the first. It's somewhat
subtle but I think useful to understand why the checker decided
to warn. Once I'm done with all my work I'd like to document this
in more detail on the Wiki.
What about overflows when either argmin or argmax won't fit into dirtype
without an overflow? What's the right behavior there?
That's fine just as long as the property above holds.
I think the algorithm works but the code came from tree-vrp where
there are all sorts of additional checks for some infinities which
VRP distinguishes from type limits like SCHAR_MIN and _MAX. I don't
fully understand those and so I'd feel better if someone who does
double checked this code.
That's what prompted my question about overflows. It was a general
concern after reading the VRP code. I did not have a specific case in
mind that would be mis-handled.
Okay.
+ tree dirmin = TYPE_MIN_VALUE (dirtype);
+ tree dirmax = TYPE_MAX_VALUE (dirtype);
From this point onward argmin/argmax are either not integers or we're
doing a narrowing conversion, right? In both cases the fact that we're
doing a narrowing conversion constrains the range
Argmin and argmax are always integers. The rest of the function
handles the case where the postcondition above doesn't hold (e.g.,
in function g above).
OK. So is the hangup really a problem in how the return type is
documented? I parsed the comment as essentially saying we return true
if the range gets adjusted in any way -- thus a sign change in the first
block would qualify, but we returned false which seemed inconsistent.
Looking at it again, what I think it's saying is we're returning true
only for a subset of adjustments to the range, ie, those cases where the
postcondition does not hold. Correct?
Correct. Would changing the description of the return value to
this make it clearer?
Return true when the range has been adjusted to the full unsigned
range of DIRTYPE, [0, DIRTYPE_MAX], false otherwise.
Martin
