On Tue, 29 Sep 2020 at 00:02, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> 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. 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
Hi Martin, One of t new tests fails on many arm configurations and on aarch64 FAIL: gcc.dg/Wstringop-overflow-47.c (test for warnings, line 29) FAIL: gcc.dg/Wstringop-overflow-47.c (test for warnings, line 32) FAIL: gcc.dg/Wstringop-overflow-47.c (test for warnings, line 37) It looks like it is also failing on x86, s390 and ia64 according to gcc-testresults. Can you check? Thanks
