On Tue, 2023-06-06 at 13:48 +0200, priour...@gmail.com wrote:
> From: Benjamin Priour <vultk...@gcc.gnu.org>
Hi Benjamin, thanks for this patch.
Overall it look great.
You didn't say what testing you did on the patch. Typically when
preparing a patch for the mailing list we do a bootstrap build of gcc
with the patch and run the regression test suite, and we state that we
did this, and which configuration we did it on. Do you have access to
a machine where this process is fast enough to not be painful? (for
reference, it takes me 2 hours for one of my patches). If not, we can
get you an account on the GCC Compile Farm. For patches that just
touch the analyzer subdirectory we can probably get away with just
testing the C, C++ and Fortran frontends and testcases.
I have some minor formatting nitpicks:
[...snip...]
>
> -/* Check whether an access is past the end of the BASE_REG. */
> +/* Check whether an access is past the end of the BASE_REG.
> + Return TRUE if the access was valid, FALSE otherwise.
> +*/
Normally we try to have the terminating "*/" on the final line of the
text of the comment, rather than starting a new line. I sometimes
break this rule for ASCII art in comments, but that isn't the case
here.
[...snip...]
> @@ -800,6 +802,7 @@ region_model::check_symbolic_bounds (const region
> *base_reg,
> offset_tree,
> num_bytes_tree,
> capacity_tree));
> + return false;
It's an unfortunate side-effect of our mixed spaces-and-tabs
indentation style that when a patch prefixes a line with "+" it's hard
for me to tell if the indentation is correct when reviewing a patch.
I'm going to assume that this is indented correctly, and it just looks
strange at review-time due to that.
Do you have "visual whitespace" turned on in your editor?
[...snip...]
>
> -/* May complain when the access on REG is out-of-bounds. */
> +/* May complain when the access on REG is out-of-bounds.
> + Return TRUE if the access was valid, FALSE otherwise.
> +*/
Same nitpick as above about the "*/" comment terminator.
>
> -void
> +bool
> region_model::check_region_bounds (const region *reg,
> enum access_direction dir,
> region_model_context *ctxt) const
[...snip...]
> @@ -929,13 +938,16 @@ region_model::check_region_bounds (const region *reg,
> case DIR_READ:
> ctxt->warn (make_unique<concrete_buffer_over_read> (reg, diag_arg,
> out,
> byte_bound));
> + oob_safe = false;
> break;
> case DIR_WRITE:
> ctxt->warn (make_unique<concrete_buffer_overflow> (reg, diag_arg,
> out,
> byte_bound));
> + oob_safe = false;
> break;
> }
As above; please doublecheck the indentation on the added lines on the
above.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
> b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
> index 2a61d8ca236..568f9cad199 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
> @@ -68,7 +68,6 @@ void test8 (size_t size, size_t offset)
> char dst[size];
> memcpy (dst, src, size + offset); /* { dg-line test8 } */
> /* { dg-warning "over-read" "warning" { target *-*-* } test8 } */
> - /* { dg-warning "use of uninitialized value" "warning" { target *-*-* }
> test8 } */
> /* { dg-warning "overflow" "warning" { target *-*-* } test8 } */
> }
>
> @@ -78,7 +77,6 @@ void test9 (size_t size, size_t offset)
> int32_t dst[size];
> memcpy (dst, src, 4 * size + 1); /* { dg-line test9 } */
> /* { dg-warning "over-read" "warning" { target *-*-* } test9 } */
> - /* { dg-warning "use of uninitialized value" "warning" { target *-*-* }
> test9 } */
> /* { dg-warning "overflow" "warning" { target *-*-* } test9 } */
> }
>
It could be argued for the above two cases that we should emit *both*
the uninit and the oob warnings, given that the part of the region
being read that's within bounds is uninitialized - but I can't see a
way to implement that without massively overcomplicating things, so
let's go with the approach in your patch.
Thanks again for the patch; sorry if this comes across as overly
nitpicky.
Dave
