Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/13/20 3:46 AM, Christophe Lyon wrote:
On Tue, 29 Sep 2020 at 00:02, Martin Sebor via Gcc-patches
wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
-bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve warnings
for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest object.
That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
-return size;
+return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On Tue, 29 Sep 2020 at 00:02, Martin Sebor via Gcc-patches
wrote:
>
> On 9/25/20 11:17 PM, Jason Merrill wrote:
> > On 9/22/20 4:05 PM, Martin Sebor wrote:
> >> The rebased and retested patches are attached.
> >>
> >> On 9/21/20 3:17 PM, Martin Sebor wrote:
> >>> Ping:
> >>> https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
> >>>
> >>> (I'm working on rebasing the patch on top of the latest trunk which
> >>> has changed some of the same code but it'd be helpful to get a go-
> >>> ahead on substance the changes. I don't expect the rebase to
> >>> require any substantive modifications.)
> >>>
> >>> Martin
> >>>
> >>> On 9/14/20 4:01 PM, Martin Sebor wrote:
> On 9/4/20 11:14 AM, Jason Merrill wrote:
> > On 9/3/20 2:44 PM, Martin Sebor wrote:
> >> On 9/1/20 1:22 PM, Jason Merrill wrote:
> >>> On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
> -Wplacement-new handles array indices and pointer offsets the same:
> by adjusting them by the size of the element. That's correct for
> the latter but wrong for the former, causing false positives when
> the element size is greater than one.
>
> In addition, the warning doesn't even attempt to handle arrays of
> arrays. I'm not sure if I forgot or if I simply didn't think of
> it.
>
> The attached patch corrects these oversights by replacing most
> of the -Wplacement-new code with a call to compute_objsize which
> handles all this correctly (plus more), and is also better tested.
> But even compute_objsize has bugs: it trips up while converting
> wide_int to offset_int for some pointer offset ranges. Since
> handling the C++ IL required changes in this area the patch also
> fixes that.
>
> For review purposes, the patch affects just the middle end.
> The C++ diff pretty much just removes code from the front end.
> >>>
> >>> The C++ changes are OK.
> >>
> >> Thank you for looking at the rest as well.
> >>
> >>>
> -compute_objsize (tree ptr, int ostype, access_ref *pref,
> -bitmap *visited, const vr_values *rvals /* =
> NULL */)
> +compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
> *visited,
> +const vr_values *rvals)
> >>>
> >>> This reformatting seems unnecessary, and I prefer to keep the
> >>> comment about the default argument.
> >>
> >> This overload doesn't take a default argument. (There was a stray
> >> declaration of a similar function at the top of the file that had
> >> one. I've removed it.)
> >
> > Ah, true.
> >
> - if (!size || TREE_CODE (size) != INTEGER_CST)
> - return false;
> >>> >...
> >>>
> >>> You change some failure cases in compute_objsize to return
> >>> success with a maximum range, while others continue to return
> >>> failure. This needs commentary about the design rationale.
> >>
> >> This is too much for a comment in the code but the background is
> >> this: compute_objsize initially returned the object size as a
> >> constant.
> >> Recently, I have enhanced it to return a range to improve warnings
> >> for
> >> allocated objects. With that, a failure can be turned into
> >> success by
> >> having the function set the range to that of the largest object.
> >> That
> >> should simplify the function's callers and could even improve
> >> the detection of some invalid accesses. Once this change is made
> >> it might even be possible to change its return type to void.
> >>
> >> The change that caught your eye is necessary to make the function
> >> a drop-in replacement for the C++ front end code which makes this
> >> same assumption. Without it, a number of test cases that exercise
> >> VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
> >>
> >>void f (int n)
> >>{
> >> char a[n];
> >> new (a - 1) int ();
> >>}
> >>
> >> Changing any of the other places isn't necessary for existing tests
> >> to pass (and I didn't want to introduce too much churn). But I do
> >> want to change the rest of the function along the same lines at some
> >> point.
> >
> > Please do change the other places to be consistent; better to have
> > more churn than to leave the function half-updated. That can be a
> > separate patch if you prefer, but let's do it now rather than later.
>
> I've made most of these changes in the other patch (also attached).
> I'm quite happy with the result but it turned out to be a lot more
> work than either of us expected, mostly due to the amount of testing.
>
> I've left a couple of failing cases in place mainly as
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/11/20 9:44 PM, Jason Merrill wrote:
On 10/11/20 6:45 PM, Martin Sebor wrote:
On 10/9/20 9:13 AM, Jason Merrill wrote:
On 10/9/20 10:51 AM, Martin Sebor wrote:
On 10/8/20 1:40 PM, Jason Merrill wrote:
On 10/8/20 3:18 PM, Martin Sebor wrote:
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the
comments with the definition as well as the in-class
declaration.
Only one access_ref member function is defined
out-of-line: offset_bounded(). I've adjusted the comment
and copied it above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded
in the new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user
with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked
and
renamed the get_range function to get_offset_range to do a
better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still
returns true iff either the range is a single value or one of the
bounds are unrepresentable in ptrdiff_t. I'm still unclear how
this corresponds to "Return true if OFFRNG is bounded to a
subrange of possible offset values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
- return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >=
wi::to_offset (max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <=
wi::to_offset (max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Ah, yes, there are two patches in that email; the first introduces
the broken offset_bounded, and the second one fixes it without
mentioning that in the ChangeLog. How about moving the fix to the
first patch?
Sure, I can do that. Anything else or is the final version okay
to commit with this adjustment?
OK with that adjustment.
I've done more testing and found a bug in the second patch: adding
an offset in an inverted range to an existing offset range isn't as
simple as adding up the bounds because they mean different things:
like an anti-range, an inverted range is a union of two subranges.
Instead, the upper bound needs to be extended to PTRDIFF_MAX because
that is the maximum being added, and the lower bound either reset to
zero if the absolute value of the maximum being added is less than
it, or incremented by the absolute value otherwise.
For example, given:
char a[8];
char *pa = a;
char *p1 = pa + i; // i's range is [3, 5]
char *p2 = p1 + j; // j's range is [1, -4]
the range of p2's offset isn't [4, 1] but [4, PTRDIFF_MAX] (or more
precisely [4, 8] if we assume it's valid). But the range of p3's
valid offset in this last pointer
char *p3 = p2 + k; // k's range is [5, -4]
is all of [0, PTRDIFF_MAX] (or, more accurately, [0, 8]).
This may seem obvious but it took me a while at first to wrap my head
around.
It makes sense, but doesn't seem obvious; a bit more comment might be nice.
I just now noticed this suggestion, after pushing both patches.
I'll keep it in mind and add something later.
I've tweaked access_ref::add_offset in the patch to handle this
correctly. The function now ensures that every offset is in
a regular range (and not an inverted one). That in turn simplifies
access_ref::size_remaining. Since an inverted range is the same as
an anti-range, there's no reason to exclude the latter anymore(*).
The diff on top of the approved patch is attached.
I've retested this new revision of the patch with Glibc and GDB/
Binutils, (the latter fails due to PR 97360), and the Linux kernel.
Please let me know if you have any questions or concerns with
this change. If not, I'd like to commit it sometime tomorrow.
Martin
[*] I was curious how often these inverted ranges/anti-ranges come
up in pointer arithmetic to see if handling them is worthwhile. I
instrumented GCC to print them in get_range() on master where they
are only looked at in calls to built-in functions, and in another
patch I'm working on where they are looked at for every pointer
addition. They accoun
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/11/20 6:45 PM, Martin Sebor wrote:
On 10/9/20 9:13 AM, Jason Merrill wrote:
On 10/9/20 10:51 AM, Martin Sebor wrote:
On 10/8/20 1:40 PM, Jason Merrill wrote:
On 10/8/20 3:18 PM, Martin Sebor wrote:
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the
comments with the definition as well as the in-class
declaration.
Only one access_ref member function is defined out-of-line:
offset_bounded(). I've adjusted the comment and copied it
above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded
in the new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user
with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked and
renamed the get_range function to get_offset_range to do a
better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still returns
true iff either the range is a single value or one of the bounds
are unrepresentable in ptrdiff_t. I'm still unclear how this
corresponds to "Return true if OFFRNG is bounded to a subrange of
possible offset values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
- return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >=
wi::to_offset (max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <=
wi::to_offset (max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Ah, yes, there are two patches in that email; the first introduces
the broken offset_bounded, and the second one fixes it without
mentioning that in the ChangeLog. How about moving the fix to the
first patch?
Sure, I can do that. Anything else or is the final version okay
to commit with this adjustment?
OK with that adjustment.
I've done more testing and found a bug in the second patch: adding
an offset in an inverted range to an existing offset range isn't as
simple as adding up the bounds because they mean different things:
like an anti-range, an inverted range is a union of two subranges.
Instead, the upper bound needs to be extended to PTRDIFF_MAX because
that is the maximum being added, and the lower bound either reset to
zero if the absolute value of the maximum being added is less than
it, or incremented by the absolute value otherwise.
For example, given:
char a[8];
char *pa = a;
char *p1 = pa + i; // i's range is [3, 5]
char *p2 = p1 + j; // j's range is [1, -4]
the range of p2's offset isn't [4, 1] but [4, PTRDIFF_MAX] (or more
precisely [4, 8] if we assume it's valid). But the range of p3's
valid offset in this last pointer
char *p3 = p2 + k; // k's range is [5, -4]
is all of [0, PTRDIFF_MAX] (or, more accurately, [0, 8]).
This may seem obvious but it took me a while at first to wrap my head
around.
It makes sense, but doesn't seem obvious; a bit more comment might be nice.
I've tweaked access_ref::add_offset in the patch to handle this
correctly. The function now ensures that every offset is in
a regular range (and not an inverted one). That in turn simplifies
access_ref::size_remaining. Since an inverted range is the same as
an anti-range, there's no reason to exclude the latter anymore(*).
The diff on top of the approved patch is attached.
I've retested this new revision of the patch with Glibc and GDB/
Binutils, (the latter fails due to PR 97360), and the Linux kernel.
Please let me know if you have any questions or concerns with
this change. If not, I'd like to commit it sometime tomorrow.
Martin
[*] I was curious how often these inverted ranges/anti-ranges come
up in pointer arithmetic to see if handling them is worthwhile. I
instrumented GCC to print them in get_range() on master where they
are only looked at in calls to built-in functions, and in another
patch I'm working on where they are looked at for every pointer
addition. They account for 16% to 23% (GCC and Glibc, respectively)
and 22% to 32% (Glibc and GCC). The detailed results are below.
GCC
Builtin pointer arithmetic:
kind
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/9/20 9:13 AM, Jason Merrill wrote:
On 10/9/20 10:51 AM, Martin Sebor wrote:
On 10/8/20 1:40 PM, Jason Merrill wrote:
On 10/8/20 3:18 PM, Martin Sebor wrote:
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the
comments with the definition as well as the in-class
declaration.
Only one access_ref member function is defined out-of-line:
offset_bounded(). I've adjusted the comment and copied it
above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded
in the new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked and
renamed the get_range function to get_offset_range to do a
better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still returns
true iff either the range is a single value or one of the bounds
are unrepresentable in ptrdiff_t. I'm still unclear how this
corresponds to "Return true if OFFRNG is bounded to a subrange of
possible offset values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
- return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >=
wi::to_offset (max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <=
wi::to_offset (max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Ah, yes, there are two patches in that email; the first introduces
the broken offset_bounded, and the second one fixes it without
mentioning that in the ChangeLog. How about moving the fix to the
first patch?
Sure, I can do that. Anything else or is the final version okay
to commit with this adjustment?
OK with that adjustment.
I've done more testing and found a bug in the second patch: adding
an offset in an inverted range to an existing offset range isn't as
simple as adding up the bounds because they mean different things:
like an anti-range, an inverted range is a union of two subranges.
Instead, the upper bound needs to be extended to PTRDIFF_MAX because
that is the maximum being added, and the lower bound either reset to
zero if the absolute value of the maximum being added is less than
it, or incremented by the absolute value otherwise.
For example, given:
char a[8];
char *pa = a;
char *p1 = pa + i; // i's range is [3, 5]
char *p2 = p1 + j; // j's range is [1, -4]
the range of p2's offset isn't [4, 1] but [4, PTRDIFF_MAX] (or more
precisely [4, 8] if we assume it's valid). But the range of p3's
valid offset in this last pointer
char *p3 = p2 + k; // k's range is [5, -4]
is all of [0, PTRDIFF_MAX] (or, more accurately, [0, 8]).
This may seem obvious but it took me a while at first to wrap my head
around.
I've tweaked access_ref::add_offset in the patch to handle this
correctly. The function now ensures that every offset is in
a regular range (and not an inverted one). That in turn simplifies
access_ref::size_remaining. Since an inverted range is the same as
an anti-range, there's no reason to exclude the latter anymore(*).
The diff on top of the approved patch is attached.
I've retested this new revision of the patch with Glibc and GDB/
Binutils, (the latter fails due to PR 97360), and the Linux kernel.
Please let me know if you have any questions or concerns with
this change. If not, I'd like to commit it sometime tomorrow.
Martin
[*] I was curious how often these inverted ranges/anti-ranges come
up in pointer arithmetic to see if handling them is worthwhile. I
instrumented GCC to print them in get_range() on master where they
are only looked at in calls to built-in functions, and in another
patch I'm working on where they are looked at for every pointer
addition. They account for 16% to 23% (GCC and Glibc, respectively)
and 22% to 32% (Glibc and GCC). The detailed results are below.
GCC
Builtin pointer arithmetic:
kind ordinary inverted
RANGE636 (38%) 150 ( 9%)
ANTI_RANGE28 ( 1%) 99 ( 6%)
VARYING 7
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/9/20 10:51 AM, Martin Sebor wrote:
On 10/8/20 1:40 PM, Jason Merrill wrote:
On 10/8/20 3:18 PM, Martin Sebor wrote:
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the
comments with the definition as well as the in-class
declaration.
Only one access_ref member function is defined out-of-line:
offset_bounded(). I've adjusted the comment and copied it above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded in
the new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked and
renamed the get_range function to get_offset_range to do a
better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still returns
true iff either the range is a single value or one of the bounds are
unrepresentable in ptrdiff_t. I'm still unclear how this
corresponds to "Return true if OFFRNG is bounded to a subrange of
possible offset values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
- return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >=
wi::to_offset (max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <=
wi::to_offset (max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Ah, yes, there are two patches in that email; the first introduces the
broken offset_bounded, and the second one fixes it without mentioning
that in the ChangeLog. How about moving the fix to the first patch?
Sure, I can do that. Anything else or is the final version okay
to commit with this adjustment?
OK with that adjustment.
Jason
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/8/20 1:40 PM, Jason Merrill wrote:
On 10/8/20 3:18 PM, Martin Sebor wrote:
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the comments
with the definition as well as the in-class declaration.
Only one access_ref member function is defined out-of-line:
offset_bounded(). I've adjusted the comment and copied it above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded in
the new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked and
renamed the get_range function to get_offset_range to do a better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still returns
true iff either the range is a single value or one of the bounds are
unrepresentable in ptrdiff_t. I'm still unclear how this corresponds
to "Return true if OFFRNG is bounded to a subrange of possible offset
values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
- return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >=
wi::to_offset (max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <=
wi::to_offset (max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Ah, yes, there are two patches in that email; the first introduces the
broken offset_bounded, and the second one fixes it without mentioning
that in the ChangeLog. How about moving the fix to the first patch?
Sure, I can do that. Anything else or is the final version okay
to commit with this adjustment?
Martin
Jason
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/8/20 3:18 PM, Martin Sebor wrote:
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the comments
with the definition as well as the in-class declaration.
Only one access_ref member function is defined out-of-line:
offset_bounded(). I've adjusted the comment and copied it above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded in
the new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked and
renamed the get_range function to get_offset_range to do a better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still returns
true iff either the range is a single value or one of the bounds are
unrepresentable in ptrdiff_t. I'm still unclear how this corresponds
to "Return true if OFFRNG is bounded to a subrange of possible offset
values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
- return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >= wi::to_offset
(max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <= wi::to_offset
(max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Ah, yes, there are two patches in that email; the first introduces the
broken offset_bounded, and the second one fixes it without mentioning
that in the ChangeLog. How about moving the fix to the first patch?
Jason
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 3:01 PM, Jason Merrill wrote:
On 10/7/20 4:11 PM, Martin Sebor wrote:
...
For the various member functions, please include the comments
with the definition as well as the in-class declaration.
Only one access_ref member function is defined out-of-line:
offset_bounded(). I've adjusted the comment and copied it above
the function definition.
And size_remaining, as quoted above?
I have this in my tree:
/* Return the maximum amount of space remaining and if non-null, set
argument to the minimum. */
I'll add it when I commit the patch.
I also don't see a comment above the definition of offset_bounded in the
new patch?
There is a comment in the latest patch.
...
The goal of conditionals is to avoid overwhelming the user with
excessive numbers that may not be meaningful or even relevant
to the warning. I've corrected the function body, tweaked and
renamed the get_range function to get_offset_range to do a better
job of extracting ranges from the types of some nonconstant
expressions the front end passes it, and added a new test for
all this. Attached is the new revision.
offset_bounded looks unchanged in the new patch. It still returns true
iff either the range is a single value or one of the bounds are
unrepresentable in ptrdiff_t. I'm still unclear how this corresponds to
"Return true if OFFRNG is bounded to a subrange of possible offset values."
I don't think you're looking at the latest patch. It has this:
+/* Return true if OFFRNG is bounded to a subrange of offset values
+ valid for the largest possible object. */
+
bool
access_ref::offset_bounded () const
{
- if (offrng[0] == offrng[1])
-return false;
-
tree min = TYPE_MIN_VALUE (ptrdiff_type_node);
tree max = TYPE_MAX_VALUE (ptrdiff_type_node);
- return offrng[0] <= wi::to_offset (min) || offrng[1] >= wi::to_offset
(max);
+ return wi::to_offset (min) <= offrng[0] && offrng[1] <= wi::to_offset
(max);
}
Here's a link to it in the archive:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200928/9026783a/attachment-0003.bin
Is this version okay to commit?
Martin
It still looks like the middle case is unreachable: if offrng[0] ==
offrng[1], offset_bounded returns false, so we don't get as far as
testing it for the middle case.
Jason
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 4:11 PM, Martin Sebor wrote:
On 10/7/20 1:28 PM, Jason Merrill wrote:
On 10/7/20 11:19 AM, Martin Sebor wrote:
On 10/7/20 9:07 AM, Jason Merrill wrote:
On 10/7/20 10:42 AM, Martin Sebor wrote:
On 10/7/20 8:26 AM, Jason Merrill wrote:
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk
which
has changed some of the same code but it'd be helpful to get a
go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets
the same:
by adjusting them by the size of the element. That's
correct for
the latter but wrong for the former, causing false
positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle
arrays of
arrays. I'm not sure if I forgot or if I simply didn't
think of
it.
The attached patch corrects these oversights by replacing
most
of the -Wplacement-new code with a call to
compute_objsize which
handles all this correctly (plus more), and is also
better tested.
But even compute_objsize has bugs: it trips up while
converting
wide_int to offset_int for some pointer offset ranges.
Since
handling the C++ IL required changes in this area the
patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front
end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals
/* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep
the comment about the default argument.
This overload doesn't take a default argument. (There was
a stray
declaration of a similar function at the top of the file
that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to
return failure. This needs commentary about the design
rationale.
This is too much for a comment in the code but the
background is
this: compute_objsize initially returned the object size as
a constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest
object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change
is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the
function
a drop-in replacement for the C++ front end code which
makes this
same assumption. Without it, a number of test cases that
exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For
example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for
existing tests
to pass (and I didn't want to introduce too much churn).
But I do
want to change the rest of the function along the same
lines at some
point.
Please do change the other places to be consistent; better
to have more churn than to leave the function half-updated.
That can be a separate patch if you prefer, but let's do it
now rather than later.
I've made most of these changes in the other patch (also
attached).
I'm quite happy with the result but it turned out to be a lot
more
work than either of us expected, mostly due to the amount of
testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the
caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think
it's necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful
but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return s
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 1:28 PM, Jason Merrill wrote:
On 10/7/20 11:19 AM, Martin Sebor wrote:
On 10/7/20 9:07 AM, Jason Merrill wrote:
On 10/7/20 10:42 AM, Martin Sebor wrote:
On 10/7/20 8:26 AM, Jason Merrill wrote:
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk
which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets
the same:
by adjusting them by the size of the element. That's
correct for
the latter but wrong for the former, causing false
positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle
arrays of
arrays. I'm not sure if I forgot or if I simply didn't
think of
it.
The attached patch corrects these oversights by replacing
most
of the -Wplacement-new code with a call to compute_objsize
which
handles all this correctly (plus more), and is also better
tested.
But even compute_objsize has bugs: it trips up while
converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the
patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front
end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals
/* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep
the comment about the default argument.
This overload doesn't take a default argument. (There was a
stray
declaration of a similar function at the top of the file
that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to
return failure. This needs commentary about the design
rationale.
This is too much for a comment in the code but the
background is
this: compute_objsize initially returned the object size as
a constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest
object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is
made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the
function
a drop-in replacement for the C++ front end code which makes
this
same assumption. Without it, a number of test cases that
exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for
existing tests
to pass (and I didn't want to introduce too much churn).
But I do
want to change the rest of the function along the same lines
at some
point.
Please do change the other places to be consistent; better to
have more churn than to leave the function half-updated.
That can be a separate patch if you prefer, but let's do it
now rather than later.
I've made most of these changes in the other patch (also
attached).
I'm quite happy with the result but it turned out to be a lot
more
work than either of us expected, mostly due to the amount of
testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the
caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think
it's necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UN
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 11:19 AM, Martin Sebor wrote:
On 10/7/20 9:07 AM, Jason Merrill wrote:
On 10/7/20 10:42 AM, Martin Sebor wrote:
On 10/7/20 8:26 AM, Jason Merrill wrote:
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets
the same:
by adjusting them by the size of the element. That's
correct for
the latter but wrong for the former, causing false
positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle
arrays of
arrays. I'm not sure if I forgot or if I simply didn't
think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize
which
handles all this correctly (plus more), and is also better
tested.
But even compute_objsize has bugs: it trips up while
converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch
also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /*
= NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep
the comment about the default argument.
This overload doesn't take a default argument. (There was a
stray
declaration of a similar function at the top of the file that
had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to
return failure. This needs commentary about the design
rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest
object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is
made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the
function
a drop-in replacement for the C++ front end code which makes
this
same assumption. Without it, a number of test cases that
exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing
tests
to pass (and I didn't want to introduce too much churn). But
I do
want to change the rest of the function along the same lines
at some
point.
Please do change the other places to be consistent; better to
have more churn than to leave the function half-updated. That
can be a separate patch if you prefer, but let's do it now
rather than later.
I've made most of these changes in the other patch (also
attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of
testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think
it's necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the com
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 9:07 AM, Jason Merrill wrote:
On 10/7/20 10:42 AM, Martin Sebor wrote:
On 10/7/20 8:26 AM, Jason Merrill wrote:
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets
the same:
by adjusting them by the size of the element. That's
correct for
the latter but wrong for the former, causing false positives
when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle
arrays of
arrays. I'm not sure if I forgot or if I simply didn't
think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize
which
handles all this correctly (plus more), and is also better
tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch
also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /*
= NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a
stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest
object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that
exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing
tests
to pass (and I didn't want to introduce too much churn). But
I do
want to change the rest of the function along the same lines
at some
point.
Please do change the other places to be consistent; better to
have more churn than to leave the function half-updated. That
can be a separate patch if you prefer, but let's do it now
rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of
testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think
it's necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing)
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 10:42 AM, Martin Sebor wrote:
On 10/7/20 8:26 AM, Jason Merrill wrote:
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the
same:
by adjusting them by the size of the element. That's correct
for
the latter but wrong for the former, causing false positives
when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle
arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better
tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest
object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing
tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at
some
point.
Please do change the other places to be consistent; better to
have more churn than to leave the function half-updated. That
can be a separate patch if you prefer, but let's do it now
rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of
testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 10/7/20 8:26 AM, Jason Merrill wrote:
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the
same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better
tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest object.
That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at
some
point.
Please do change the other places to be consistent; better to
have more churn than to leave the function half-updated. That
can be a separate patch if you prefer, but let's do it now rather
than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 9/28/20 6:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the
same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest object.
That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either ha
[PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555019.html
On 9/28/20 4:01 PM, Martin Sebor wrote:
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the
same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref,
bitmap *visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve
warnings for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest object.
That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of t
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 9/25/20 11:17 PM, Jason Merrill wrote:
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return
success with a maximum range, while others continue to return
failure. This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve warnings
for
allocated objects. With that, a failure can be turned into
success by
having the function set the range to that of the largest object.
That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its s
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 9/22/20 4:05 PM, Martin Sebor wrote:
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* =
NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success
with a maximum range, while others continue to return failure.
This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a
constant.
Recently, I have enhanced it to return a range to improve warnings for
allocated objects. With that, a failure can be turned into success by
having the function set the range to that of the largest object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its size, ...
OK, I see it now.
It handles a nu
Re: [PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
The rebased and retested patches are attached.
On 9/21/20 3:17 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* = NULL
*/)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success
with a maximum range, while others continue to return failure.
This needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a constant.
Recently, I have enhanced it to return a range to improve warnings for
allocated objects. With that, a failure can be turned into success by
having the function set the range to that of the largest object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its size, ...
OK, I see it now.
It handles a number of cases in Wplacement-new-size.C fail
[PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
(I'm working on rebasing the patch on top of the latest trunk which
has changed some of the same code but it'd be helpful to get a go-
ahead on substance the changes. I don't expect the rebase to
require any substantive modifications.)
Martin
On 9/14/20 4:01 PM, Martin Sebor wrote:
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the
comment about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success
with a maximum range, while others continue to return failure. This
needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a constant.
Recently, I have enhanced it to return a range to improve warnings for
allocated objects. With that, a failure can be turned into success by
having the function set the range to that of the largest object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have
more churn than to leave the function half-updated. That can be a
separate patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its size, ...
OK, I see it now.
It handles a number of cases in Wplacement-new-size.C fail that
construct a larger object in an extern declaration of a template,
like this:
tem
Re: [PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 9/4/20 11:14 AM, Jason Merrill wrote:
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the comment
about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success
with a maximum range, while others continue to return failure. This
needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a constant.
Recently, I have enhanced it to return a range to improve warnings for
allocated objects. With that, a failure can be turned into success by
having the function set the range to that of the largest object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have more
churn than to leave the function half-updated. That can be a separate
patch if you prefer, but let's do it now rather than later.
I've made most of these changes in the other patch (also attached).
I'm quite happy with the result but it turned out to be a lot more
work than either of us expected, mostly due to the amount of testing.
I've left a couple of failing cases in place mainly as reminders
to handle them better (which means I also didn't change the caller
to avoid testing for failures). I've also added TODO notes with
reminders to handle some of the new codes more completely.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its size, ...
OK, I see it now.
It handles a number of cases in Wplacement-new-size.C fail that
construct a larger object in an extern declaration of a template,
like this:
template struct S { char c; };
extern S s;
void f ()
{
new (&s) int ();
}
I don't know why DECL_SIZE isn't set here (I don't think it can
be anything but equal to TYPE_SIZE, can it?) and other than struct
objects with a flexible array member where this identity doesn't
hold I can't think of others. Am I missing something?
Good question. The
Re: [PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 9/3/20 2:44 PM, Martin Sebor wrote:
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the comment
about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
Ah, true.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success
with a maximum range, while others continue to return failure. This
needs commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a constant.
Recently, I have enhanced it to return a range to improve warnings for
allocated objects. With that, a failure can be turned into success by
having the function set the range to that of the largest object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
Please do change the other places to be consistent; better to have more
churn than to leave the function half-updated. That can be a separate
patch if you prefer, but let's do it now rather than later.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's
necessary to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its size, ...
OK, I see it now.
It handles a number of cases in Wplacement-new-size.C fail that
construct a larger object in an extern declaration of a template,
like this:
template struct S { char c; };
extern S s;
void f ()
{
new (&s) int ();
}
I don't know why DECL_SIZE isn't set here (I don't think it can
be anything but equal to TYPE_SIZE, can it?) and other than struct
objects with a flexible array member where this identity doesn't
hold I can't think of others. Am I missing something?
Good question. The attached patch should fix that, so you shouldn't
need the change to decl_init_size:
commit 95c284379d67efb79a30273236fd6769a12f3031
Author: Jason Merrill
Date: Fri Sep 4 12:14:19 2020 -0400
layout
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 31d68745844..f6ca51ad8ba 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -17444,10 +17444,10 @@ complete_vars (tree type)
&& (TYPE_MAIN_VARIANT (strip_array_types (type))
== iv->incomplete_type))
{
- /* Complete the type
Re: [PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 9/1/20 1:22 PM, Jason Merrill wrote:
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
Thank you for looking at the rest as well.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
*visited,
+ const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the comment
about the default argument.
This overload doesn't take a default argument. (There was a stray
declaration of a similar function at the top of the file that had
one. I've removed it.)
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success with
a maximum range, while others continue to return failure. This needs
commentary about the design rationale.
This is too much for a comment in the code but the background is
this: compute_objsize initially returned the object size as a constant.
Recently, I have enhanced it to return a range to improve warnings for
allocated objects. With that, a failure can be turned into success by
having the function set the range to that of the largest object. That
should simplify the function's callers and could even improve
the detection of some invalid accesses. Once this change is made
it might even be possible to change its return type to void.
The change that caught your eye is necessary to make the function
a drop-in replacement for the C++ front end code which makes this
same assumption. Without it, a number of test cases that exercise
VLAs fail in g++.dg/warn/Wplacement-new-size-5.C. For example:
void f (int n)
{
char a[n];
new (a - 1) int ();
}
Changing any of the other places isn't necessary for existing tests
to pass (and I didn't want to introduce too much churn). But I do
want to change the rest of the function along the same lines at some
point.
I've tweaked the comment a little bit without going into all this
detail.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's necessary
to initialize it at the declaration.
I find initializing pass-by-pointer local variables helpful but
I don't insist on it.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
- return size;
+ return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
By my reading (and writing) the change is covered by the first
sentence:
Returns the size of the object designated by DECL considering
its initializer if it either has one or if it would not affect
its size, ...
It handles a number of cases in Wplacement-new-size.C fail that
construct a larger object in an extern declaration of a template,
like this:
template struct S { char c; };
extern S s;
void f ()
{
new (&s) int ();
}
I don't know why DECL_SIZE isn't set here (I don't think it can
be anything but equal to TYPE_SIZE, can it?) and other than struct
objects with a flexible array member where this identity doesn't
hold I can't think of others. Am I missing something?
Attached is an updated patch rebased on top of the current trunk
and retested on x86_64-linux + by building Glibc.
Martin
Correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
Resolves:
PR c++/96511 - Incorrect -Wplacement-new on POINTER_PLUS into an array with 4-byte elements
PR middle-end/96384 - bogus -Wstringop-overflow= storing into multidimensional array with index in range
gcc/ChangeLog:
PR c++/96511
PR middle-end/96384
* builtins.c (get_range): Return full range of type when neither
value nor its range is available. Fail for ranges inverted due
to the signedness of offsets.
(compute_objsize): Handle more special array members. Handle
POINTER_PLUS_EXPR and VIEW_CONVERT_EXPR that come u
Re: [PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
The C++ changes are OK.
-compute_objsize (tree ptr, int ostype, access_ref *pref,
-bitmap *visited, const vr_values *rvals /* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap *visited,
+const vr_values *rvals)
This reformatting seems unnecessary, and I prefer to keep the comment
about the default argument.
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
>...
You change some failure cases in compute_objsize to return success with
a maximum range, while others continue to return failure. This needs
commentary about the design rationale.
+ special_array_member sam{ };
sam is always set by component_ref_size, so I don't think it's necessary
to initialize it at the declaration.
@@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
tree last_type = TREE_TYPE (last);
if (TREE_CODE (last_type) != ARRAY_TYPE
|| TYPE_SIZE (last_type))
-return size;
+return size ? size : TYPE_SIZE_UNIT (type);
This change seems to violate the comment for the function.
Jason
[PING 2][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551783.html On 8/19/20 9:00 AM, Martin Sebor wrote: Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551783.html On 8/11/20 10:19 AM, Martin Sebor wrote: -Wplacement-new handles array indices and pointer offsets the same: by adjusting them by the size of the element. That's correct for the latter but wrong for the former, causing false positives when the element size is greater than one. In addition, the warning doesn't even attempt to handle arrays of arrays. I'm not sure if I forgot or if I simply didn't think of it. The attached patch corrects these oversights by replacing most of the -Wplacement-new code with a call to compute_objsize which handles all this correctly (plus more), and is also better tested. But even compute_objsize has bugs: it trips up while converting wide_int to offset_int for some pointer offset ranges. Since handling the C++ IL required changes in this area the patch also fixes that. For review purposes, the patch affects just the middle end. The C++ diff pretty much just removes code from the front end. Tested on x86_64-linux plus by building the latest Glibc and confirming no new warnings. Martin
[PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551783.html On 8/11/20 10:19 AM, Martin Sebor wrote: -Wplacement-new handles array indices and pointer offsets the same: by adjusting them by the size of the element. That's correct for the latter but wrong for the former, causing false positives when the element size is greater than one. In addition, the warning doesn't even attempt to handle arrays of arrays. I'm not sure if I forgot or if I simply didn't think of it. The attached patch corrects these oversights by replacing most of the -Wplacement-new code with a call to compute_objsize which handles all this correctly (plus more), and is also better tested. But even compute_objsize has bugs: it trips up while converting wide_int to offset_int for some pointer offset ranges. Since handling the C++ IL required changes in this area the patch also fixes that. For review purposes, the patch affects just the middle end. The C++ diff pretty much just removes code from the front end. Tested on x86_64-linux plus by building the latest Glibc and confirming no new warnings. Martin
[PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
-Wplacement-new handles array indices and pointer offsets the same:
by adjusting them by the size of the element. That's correct for
the latter but wrong for the former, causing false positives when
the element size is greater than one.
In addition, the warning doesn't even attempt to handle arrays of
arrays. I'm not sure if I forgot or if I simply didn't think of
it.
The attached patch corrects these oversights by replacing most
of the -Wplacement-new code with a call to compute_objsize which
handles all this correctly (plus more), and is also better tested.
But even compute_objsize has bugs: it trips up while converting
wide_int to offset_int for some pointer offset ranges. Since
handling the C++ IL required changes in this area the patch also
fixes that.
For review purposes, the patch affects just the middle end.
The C++ diff pretty much just removes code from the front end.
Tested on x86_64-linux plus by building the latest Glibc and
confirming no new warnings.
Martin
Correct handling of indices into arrays with elements larger than 1 (PR c++/96511)
Resolves:
PR c++/96511 - Incorrect -Wplacement-new on POINTER_PLUS into an array with 4-byte elements
PR middle-end/96561 - missing warning for buffer overflow with negative offset
PR middle-end/96384 - bogus -Wstringop-overflow= storing into multidimensional array with index in range
gcc/ChangeLog:
PR c++/96511
PR middle-end/96384
* builtins.c (get_range): Return full range of type when neither
value nor its range is available. Fail for ranges inverted due
to the signedness of offsets.
(compute_objsize): Handle more special array members. Handle
POINTER_PLUS_EXPR and VIEW_CONVERT_EXPR that come up in front end
code.
(access_ref::offset_bounded): Define new member function.
* builtins.h (access_ref::eval): New data member.
(access_ref::offset_bounded): New member function.
(access_ref::offset_zero): New member function.
(compute_objsize): Declare a new overload.
* gimple-array-bounds.cc (array_bounds_checker::check_array_ref): Use
enum special_array_member.
* tree-object-size.c (decl_init_size): Return the size of the structure
type if the decl size is null.
* tree.c (component_ref_size): Use special_array_member.
* tree.h (special_array_member): Define a new type.
(component_ref_size): Change signature/
gcc/cp/ChangeLog:
PR c++/96511
PR middle-end/96384
* init.c (warn_placement_new_too_small): Call builtin_objsize instead
of duplicating what it does.
gcc/testsuite/ChangeLog:
PR c++/96511
PR middle-end/96384
* g++.dg/warn/Wplacement-new-size-1.C: Relax warnings.
* g++.dg/warn/Wplacement-new-size-2.C: Same.
* g++.dg/warn/Wplacement-new-size-6.C: Same.
* g++.dg/warn/Wplacement-new-size-7.C: New test.
* gcc.dg/Wstringop-overflow-40.c: New test.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index beb56e06d8a..4f34a99c2f9 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3977,13 +3977,32 @@ static bool
get_range (tree x, signop sgn, offset_int r[2],
const vr_values *rvals /* = NULL */)
{
+ tree type = TREE_TYPE (x);
+ if (TREE_CODE (x) != INTEGER_CST
+ && TREE_CODE (x) != SSA_NAME)
+{
+ if (TYPE_UNSIGNED (type))
+ {
+ if (sgn == SIGNED)
+ type = signed_type_for (type);
+ }
+ else if (sgn == UNSIGNED)
+ type = unsigned_type_for (type);
+
+ r[0] = wi::to_offset (TYPE_MIN_VALUE (type));
+ r[1] = wi::to_offset (TYPE_MAX_VALUE (type));
+ return x;
+}
+
wide_int wr[2];
if (!get_range (x, wr, rvals))
return false;
r[0] = offset_int::from (wr[0], sgn);
r[1] = offset_int::from (wr[1], sgn);
- return true;
+ /* Succeed only for valid ranges (pointer offsets are represented
+ as unsigned despite taking on "negative" values). */
+ return r[0] <= r[1];
}
/* Helper to compute the size of the object referenced by the PTR
@@ -4001,9 +4020,11 @@ get_range (tree x, signop sgn, offset_int r[2],
to influence code generation or optimization. */
static bool
-compute_objsize (tree ptr, int ostype, access_ref *pref,
- bitmap *visited, const vr_values *rvals /* = NULL */)
+compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap *visited,
+ const vr_values *rvals)
{
+ STRIP_NOPS (ptr);
+
const bool addr = TREE_CODE (ptr) == ADDR_EXPR;
if (addr)
ptr = TREE_OPERAND (ptr, 0);
@@ -4015,12 +4036,15 @@ compute_objsize (tree ptr, int ostype, access_ref *pref,
if (!addr && POINTER_TYPE_P (TREE_TYPE (ptr)))
return false;
- tree size = decl_init_size (ptr, false);
- if (!size || TREE_CODE (size) != INTEGER_CST)
- return false;
-
pref->ref = ptr;
- pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
+ if (tree size = decl_init_size (ptr, false))
+ if (TREE_CODE (size) == INTEGER_CST)
+ {
+ pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
+ return true;
+ }
+ pref->sizrng[0] = 0;
+ pref->sizrng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
r
