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_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 <class> struct S { char c; };
extern S<int> 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:
I've integrated it into the bug fix.
Besides the usual x86_64-linux bootstrap/regtest I tested both
patches by building a few packages, including Binutils/GDB,
Glibc,
and verifying no new warnings show up.
Martin
+offset_int
+access_ref::size_remaining (offset_int *pmin /* = NULL */) const
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.
+ if (offrng[1] < offrng[0])
What does it mean for the max offset to be less than the min
offset? I wouldn't expect that to ever happen with wide integers.
The offset is represented in sizetype with negative values
represented
as large positive values, but has to be converted to ptrdiff_t.
It looks to me like the offset is offset_int, which is both signed
and big enough to hold all values of sizetype without turning large
positive values into negative values. Where are these
sign-switching conversions happening?
In get_offset_range in builtins.c.
Since we're converting to offset_int there, why not give the
offset_int the real value rather than a bogus negative value?
I don't understand the question: the real offset (in the program)
is negative when its sizetype representation is greater than
PTRDIFF_MAX. It's worked this way for years.
OK, then we're back to my original question: why is the max offset less
than the min offset? If the range includes negative values, why isn't
the more-negative one the minimum?
I showed a simple example where it happens:
extern char a[2];
void f (unsigned long i)
{
if (i == 0)
return;
a[i] = 0; // i's range is [1, -1] (i.e., [1, SIZE_MAX])
}
The "negative" offset can't be the minimum because it would include
zero which is the one value the range excludes. It's effectively
an anti-range represented as an inverted range.
What is your concern with this? That something isn't right? Do
you want me to do something differently? The code will change
again as soon as we introduce the new Ranger API into it. I'd
like to make progress on it (and other things that depend on it)
but I've been holding off until this is approved. I believe it's
a good improvement already but it's far from the last word.
Martin
These
cases come up when the unsigned offset is an ordinary range that
corresponds to an anti-range, such as here:
extern char a[2];
void f (unsigned long i)
{
if (i == 0)
return;
a[i] = 0; // i's range is [1, -1] (i.e., [1, SIZE_MAX]
}
+ /* Return true if OFFRNG is bounded to a subrange of possible
offset
+ values. */
+ bool offset_bounded () const;
I don't understand how you're using this. The implementation
checks for the possible offset values falling outside those
representable by ptrdiff_t, unless the range is only a single
value. And then the only use is
+ if (ref.offset_zero () || !ref.offset_bounded ())
+ inform (DECL_SOURCE_LOCATION (ref.ref),
+ "%qD declared here", ref.ref);
+ else if (ref.offrng[0] == ref.offrng[1])
+ inform (DECL_SOURCE_LOCATION (ref.ref),
+ "at offset %wi from %qD declared here",
+ ref.offrng[0].to_shwi (), ref.ref);
+ else
+ inform (DECL_SOURCE_LOCATION (ref.ref),
+ "at offset [%wi, %wi] from %qD declared here",
+ ref.offrng[0].to_shwi (), ref.offrng[1].to_shwi (),
ref.ref);
So if the possible offsets are all representable by ptrdiff_t, we
don't print the range? The middle case also looks unreachable,
since offset_bounded will return false in that case.
The function was originally named "offset_unbounded." I changed
it to "offset_bounded" but looks like I didn't finish the job or
add any tests for it.
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.
Martin
