This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] handle integer overflow/wrapping in printf directives (PR 78622)
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.
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.
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.
+
+ 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?
+
+ if (TYPE_UNSIGNED (dirtype))
+ {
+ *argmin = dirmin;
+ *argmax = dirmax;
+ }
+ else
+ {
+ *argmin = integer_zero_node;
+ *argmax = dirmin;
+ }
Should this be dirmax? Am I missing something here?
It's dirmin because for a signed type, due to the sign, it results
in more bytes on output than dirmin would. (E.g., with SCHAR_MIN
= -128 and SCHAR_MAX = 127 it's four bytes vs three.)
Ah. I understand that. THanks for clarifying.
jeff