On 03/23/2016 03:47 PM, Martin Sebor wrote:
Thanks for the comments.
2) It hardwires a rather arbitrarily restrictive limit of 64 KB
on the size of the biggest C++ VLA. (This could stand to be
improved and made more intelligent, and perhaps integrated
with stack checking via -fstack-limit, after the GCC 6
release.)
The bounds checking should share code with build_new_1.
I agree that sharing the same code is right long term approach.
I had initially started out down that path, by factoring out code
from build_new_1 into a general function that I had thought could
be shared between it and cp_finish_decl. But I ended up abandoning
that approach two reasons:
a) The checking expression built for array new is quite a bit less
involved because only the major dimension of the array in requires
runtime checking (the others must be constant and are checked at
compile time). In contrast, all VLA dimensions are potentially
dynamic and so must be checked at runtime.
b) While (a) can be solved by making the checking function smart
and general enough, it felt too intrusive and potentially risky
to change array new at this stage.
That said, I'm happy to do this refactoring in stage 1.
Fair enough. I don't think we can impose an arbitrary 64K limit,
however, as that is a lot smaller than the 8MB default stack size, and
programs can use setrlimit to increase the stack farther. For GCC 6 let
not impose any limit beyond non-negative/overflowing, and as you say we
can do something better in GCC 7.
3) By throwing an exception for erroneous VLAs the patch largely
defeats the VLA Sanitizer. The sanitizer is still useful in
C++ 98 mode where the N3639 VLA runtime checking is disabled,
and when exceptions are disabled via -fno-exceptions.
Disabling the VLA checking in C++ 98 mode doesn't seem like
a useful feature, but I didn't feel like reverting what was
a deliberate decision.
What deliberate decision? The old code checked for C++14 mode because
the feature was part of the C++14 working paper. What's the rationale
for C++11 as the cutoff?
Sorry, I had misremembered the original C++ 14 check as one for
C++ 11. Are you suggesting to restore the checking only for C++
14 mode, or for all modes? Either is easy enough to do though,
IMO, it should be safe in all modes and I would expect it to be
preferable to undefined behavior.
I think all modes.
+ /* Iterate over all non-empty initializers in this array, recursively
+ building expressions to see if the elements of each are in excess
+ of the (runtime) bounds of of the array type. */
+ FOR_EACH_VEC_SAFE_ELT (v, i, ce)
+ {
+ if (1) // !vec_safe_is_empty (CONSTRUCTOR_ELTS (ce->value)))
+ {
+ tree subcheck = build_vla_check (TREE_TYPE (type),
+ ce->value,
+ vlasize, max_vlasize,
+ cst_size);
+ check = fold_build2 (TRUTH_OR_EXPR, boolean_type_node,
+ check, subcheck);
+ }
+ }
The if (1) seems left over from development.
It looks like this will multiply *cst_size by the sub-array length once
for each element of the outer array, leading to a too-large result. And
also generate redundant code to check the bounds of the sub-array
multiple times.
It seems to me that we want use the existing check for excess
initializers in build_vec_init, in the if (length_check) section, though
as you mention in 70019 that needs to be improved to handle STRING_CST.
Also, I think we should check for invalid bounds in
compute_array_index_type, next to the UBsan code. Checking bounds only
from cp_finish_decl means that we don't check uses of VLA types other
than variable declarations.
+ /* Avoid instrumenting constexpr functions. Those must
+ be checked statically, and the (non-constexpr) dynamic
+ instrumentation would cause them to be rejected. */
Hmm, this sounds wrong; constexpr functions can also be called with
non-constant arguments, and the instrumentation should be folded away
when evaluating a call with constant arguments.
Jason
