Message ID | a47e256b-11ce-7818-8391-8921ae114a99@gmail.com |
---|---|
State | New |
Headers | show |
Series | correct handling of indices into arrays with elements larger than 1 (PR c++/96511) | expand |
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: 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 >
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
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 <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? Attached is an updated patch rebased on top of the current trunk and retested on x86_64-linux + by building Glibc. Martin
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 <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:
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
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
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 >
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. > + 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. > + /* 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. Jason
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
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 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
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? > 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
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. 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 >
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? >>> 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 >> >
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. 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 >>> >> >
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? >>>>> 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 >>>> >>> >> >
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 >>>>> >>>> >>> >> >
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 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. And size_remaining, as quoted above? I also don't see a comment above the definition of offset_bounded in the new patch? >>>>>>>>> + 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. So if the excluded value was 1, the range would be [2, 0]? OK, then the answer to my question is "it indicates an anti-range" and the sign wrapping was just a distraction. >>>>>>> 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. 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." 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
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 >
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
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 >
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
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 RANGE 636 (38%) 150 ( 9%) ANTI_RANGE 28 ( 1%) 99 ( 6%) VARYING 733 (44%) total 1397 (84%) 249 (15%) All pointer arithmetic: kind ordinary inverted RANGE 4663 (45%) 2945 (28%) ANTI_RANGE 134 ( 1%) 119 ( 1%) VARYING 2344 (22%) total 7141 (69%) 3064 (30%) Glibc Builtin pointer arithmetic: kind ordinary inverted RANGE 488 (37%) 191 (14%) ANTI_RANGE 18 ( 1%) 112 ( 8%) VARYING 509 (38%) total 1015 (77%) 303 (22%) All pointer arithmetic: kind ordinary inverted RANGE 1941 (51%) 636 (16%) ANTI_RANGE 41 ( 1%) 202 ( 5%) VARYING 952 (25%) total 2934 (77%) 838 (22%)
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 ordinary inverted > RANGE 636 (38%) 150 ( 9%) > ANTI_RANGE 28 ( 1%) 99 ( 6%) > VARYING 733 (44%) > total 1397 (84%) 249 (15%) > > All pointer arithmetic: > kind ordinary inverted > RANGE 4663 (45%) 2945 (28%) > ANTI_RANGE 134 ( 1%) 119 ( 1%) > VARYING 2344 (22%) > total 7141 (69%) 3064 (30%) > > Glibc > Builtin pointer arithmetic: > kind ordinary inverted > RANGE 488 (37%) 191 (14%) > ANTI_RANGE 18 ( 1%) 112 ( 8%) > VARYING 509 (38%) > total 1015 (77%) 303 (22%) > > All pointer arithmetic: > kind ordinary inverted > RANGE 1941 (51%) 636 (16%) > ANTI_RANGE 41 ( 1%) 202 ( 5%) > VARYING 952 (25%) > total 2934 (77%) 838 (22%) > + /* When referencing a known object check to see if the offset computed > + so far is in bounds... */ ...but you don't do anything if it isn't in bounds? That could use a comment, at least. Jason
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 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 >> RANGE 636 (38%) 150 ( 9%) >> ANTI_RANGE 28 ( 1%) 99 ( 6%) >> VARYING 733 (44%) >> total 1397 (84%) 249 (15%) >> >> All pointer arithmetic: >> kind ordinary inverted >> RANGE 4663 (45%) 2945 (28%) >> ANTI_RANGE 134 ( 1%) 119 ( 1%) >> VARYING 2344 (22%) >> total 7141 (69%) 3064 (30%) >> >> Glibc >> Builtin pointer arithmetic: >> kind ordinary inverted >> RANGE 488 (37%) 191 (14%) >> ANTI_RANGE 18 ( 1%) 112 ( 8%) >> VARYING 509 (38%) >> total 1015 (77%) 303 (22%) >> >> All pointer arithmetic: >> kind ordinary inverted >> RANGE 1941 (51%) 636 (16%) >> ANTI_RANGE 41 ( 1%) 202 ( 5%) >> VARYING 952 (25%) >> total 2934 (77%) 838 (22%) > >> + /* When referencing a known object check to see if the offset computed >> + so far is in bounds... */ > > ...but you don't do anything if it isn't in bounds? That could use a > comment, at least. When the offset is out of bounds it has to stay unchanged so that the access to the object can be diagnosed and the invalid offset mentioned in the note after the warning. This also means that subsequent pointer addition can make an invalid offset valid again, preventing the access warning. A different warning will have to detect that at the point of the pointer addition (I'm about to submit a change that implements that). I added a brief comment mentioning this. Martin
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
On 10/13/20 3:46 AM, Christophe Lyon wrote: > 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? Yes, I'm tracking the problem in pr97027, but I didn't realize the test was susceptible to this dependency when I wrote it. It could be adjusted to avoid the missing warnings but the right fix is to improve the detection. Let me see if I can quickly handle the latter. In not, I'll constrain the test to just the targets where I know is passes. Thanks 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)); return true; } @@ -4028,13 +4052,13 @@ compute_objsize (tree ptr, int ostype, access_ref *pref, if (code == COMPONENT_REF) { + tree ref = TREE_OPERAND (ptr, 0); tree field = TREE_OPERAND (ptr, 1); if (ostype == 0) { /* For raw memory functions like memcpy bail if the size of the enclosing object cannot be determined. */ - tree ref = TREE_OPERAND (ptr, 0); if (!compute_objsize (ref, ostype, pref, visited, rvals) || !pref->ref) return false; @@ -4056,20 +4080,28 @@ compute_objsize (tree ptr, int ostype, access_ref *pref, return false; pref->ref = field; - /* Only return constant sizes for now while callers depend - on it. INT0LEN is true for interior zero-length arrays. */ - bool int0len = false; - tree size = component_ref_size (ptr, &int0len); - if (int0len) + + /* SAM is set for array members that might need special treatment. */ + special_array_member sam{ }; + tree size = component_ref_size (ptr, &sam); + if (sam == special_array_member::int_0) + pref->sizrng[0] = pref->sizrng[1] = 0; + else if (!pref->trail1special && sam == special_array_member::trail_1) + pref->sizrng[0] = pref->sizrng[1] = 1; + else if (size && TREE_CODE (size) == INTEGER_CST) + pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size); + else { - pref->sizrng[0] = pref->sizrng[1] = 0; - return true; + /* When the size of the member is unknown it's either a flexible + array member or a trailing special array member (either zero + length or one-element). Set the size to the maximum minus + the constant size of the type. */ + pref->sizrng[0] = 0; + pref->sizrng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node)); + if (tree recsize = TYPE_SIZE_UNIT (TREE_TYPE (ref))) + if (TREE_CODE (recsize) == INTEGER_CST) + pref->sizrng[1] -= wi::to_offset (recsize); } - - if (!size || TREE_CODE (size) != INTEGER_CST) - return false; - - pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size); return true; } @@ -4099,7 +4131,7 @@ compute_objsize (tree ptr, int ostype, access_ref *pref, return false; offset_int orng[2]; - tree off = TREE_OPERAND (ptr, 1); + tree off = pref->eval (TREE_OPERAND (ptr, 1)); if (!get_range (off, SIGNED, orng, rvals)) /* Fail unless the size of the object is zero. */ return pref->sizrng[0] == 0 && pref->sizrng[0] == pref->sizrng[1]; @@ -4129,11 +4161,20 @@ compute_objsize (tree ptr, int ostype, access_ref *pref, if (ostype && TREE_CODE (eltype) == ARRAY_TYPE) { - /* Execpt for the permissive raw memory functions which - use the size of the whole object determined above, - use the size of the referenced array. */ - pref->sizrng[0] = pref->offrng[0] + orng[0] + sz; - pref->sizrng[1] = pref->offrng[1] + orng[1] + sz; + /* Except for the permissive raw memory functions which use + the size of the whole object determined above, use the size + of the referenced array. Because the overall offset is from + the beginning of the complete array object add this overall + offset to the size of array. */ + const offset_int sizrng[2] = + { + pref->offrng[0] + orng[0] + sz, + pref->offrng[1] + orng[1] + sz + }; + if (sizrng[0] >= 0 && sizrng[0] <= pref->sizrng[0]) + pref->sizrng[0] = sizrng[0]; + if (sizrng[1] >= 0 && sizrng[1] <= pref->sizrng[1]) + pref->sizrng[1] = sizrng[1]; } } @@ -4142,6 +4183,28 @@ compute_objsize (tree ptr, int ostype, access_ref *pref, return true; } + else if (code == POINTER_PLUS_EXPR) + { + tree ref = TREE_OPERAND (ptr, 0); + if (!compute_objsize (ref, ostype, pref, visited, rvals)) + return false; + + offset_int orng[2]; + tree off = pref->eval (TREE_OPERAND (ptr, 1)); + if (!get_range (off, SIGNED, orng, rvals)) + /* Fail unless the size of the object is zero. */ + return pref->sizrng[0] == 0 && pref->sizrng[0] == pref->sizrng[1]; + + pref->offrng[0] += orng[0]; + pref->offrng[1] += orng[1]; + + return true; + } + else if (code == VIEW_CONVERT_EXPR) + { + ptr = TREE_OPERAND (ptr, 0); + return compute_objsize (ptr, ostype, pref, visited, rvals); + } if (TREE_CODE (ptr) == SSA_NAME) { @@ -4214,9 +4277,9 @@ compute_objsize (tree ptr, int ostype, access_ref *pref, /* Convenience wrapper around the above. */ -static tree +tree compute_objsize (tree ptr, int ostype, access_ref *pref, - const vr_values *rvals = NULL) + const vr_values *rvals /* = NULL */) { bitmap visited = NULL; @@ -12076,3 +12139,14 @@ builtin_with_linkage_p (tree decl) } return false; } + +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); +} diff --git a/gcc/builtins.h b/gcc/builtins.h index 8b812ceb2c4..a60998d884b 100644 --- a/gcc/builtins.h +++ b/gcc/builtins.h @@ -133,13 +133,6 @@ extern tree fold_call_stmt (gcall *, bool); extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); extern bool is_simple_builtin (tree); extern bool is_inexpensive_builtin (tree); - -class vr_values; -tree gimple_call_alloc_size (gimple *, wide_int[2] = NULL, - const vr_values * = NULL); -extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL, - const vr_values * = NULL); - extern bool readonly_data_expr (tree exp); extern bool init_target_chars (void); extern unsigned HOST_WIDE_INT target_newline; @@ -160,7 +153,8 @@ extern bool builtin_with_linkage_p (tree); /* Describes a reference to an object used in an access. */ struct access_ref { - access_ref (): ref () + access_ref () + : ref (), eval ([](tree x){ return x; }), trail1special (true) { /* Set to valid. */ offrng[0] = offrng[1] = 0; @@ -174,6 +168,22 @@ struct access_ref /* Range of offsets into and sizes of the object(s). */ offset_int offrng[2]; offset_int sizrng[2]; + + /* Return true if OFFRNG is the constant zero. */ + bool offset_zero () const + { + return offrng[0] == 0 && offrng[1] == 0; + } + + /* Return true if OFFRNG is bounded to a subrange of possible offset + values. */ + bool offset_bounded () const; + + /* Used to fold integer expressions when called from front ends. */ + tree (*eval)(tree); + /* Set if trailing one-element arrays should be treated as flexible + array members. */ + bool trail1special; }; /* Describes a pair of references used in an access by built-in @@ -184,6 +194,12 @@ struct access_data access_ref dst, src; }; +class vr_values; +tree gimple_call_alloc_size (gimple *, wide_int[2] = NULL, + const vr_values * = NULL); +extern tree compute_objsize (tree, int, access_ref *, const vr_values * = NULL); +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL, + const vr_values * = NULL); extern bool check_access (tree, tree, tree, tree, tree, tree, tree, bool = true, const access_data * = NULL); diff --git a/gcc/cp/init.c b/gcc/cp/init.c index 3f089404cf2..d544e02b1d0 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3. If not see #include "attribs.h" #include "asan.h" #include "stor-layout.h" +#include "builtins.h" static bool begin_init_stmts (tree *, tree *); static tree finish_init_stmts (bool, tree, tree); @@ -2551,27 +2552,6 @@ throw_bad_array_new_length (void) return build_cxx_call (fn, 0, NULL, tf_warning_or_error); } -/* Attempt to find the initializer for flexible array field T in the - initializer INIT, when non-null. Returns the initializer when - successful and NULL otherwise. */ -static tree -find_flexarray_init (tree t, tree init) -{ - if (!init || init == error_mark_node) - return NULL_TREE; - - unsigned HOST_WIDE_INT idx; - tree field, elt; - - /* Iterate over all top-level initializer elements. */ - FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init), idx, field, elt) - /* If the member T is found, return it. */ - if (field == t) - return elt; - - return NULL_TREE; -} - /* Attempt to verify that the argument, OPER, of a placement new expression refers to an object sufficiently large for an object of TYPE or an array of NELTS of such objects when NELTS is non-null, and issue a warning when @@ -2588,17 +2568,6 @@ warn_placement_new_too_small (tree type, tree nelts, tree size, tree oper) { location_t loc = cp_expr_loc_or_input_loc (oper); - /* The number of bytes to add to or subtract from the size of the provided - buffer based on an offset into an array or an array element reference. - Although intermediate results may be negative (as in a[3] - 2) a valid - final result cannot be. */ - offset_int adjust = 0; - /* True when the size of the entire destination object should be used - to compute the possibly optimistic estimate of the available space. */ - bool use_obj_size = false; - /* True when the reference to the destination buffer is an ADDR_EXPR. */ - bool addr_expr = false; - STRIP_NOPS (oper); /* Using a function argument or a (non-array) variable as an argument @@ -2612,231 +2581,91 @@ warn_placement_new_too_small (tree type, tree nelts, tree size, tree oper) /* Evaluate any constant expressions. */ size = fold_non_dependent_expr (size); - /* Handle the common case of array + offset expression when the offset - is a constant. */ - if (TREE_CODE (oper) == POINTER_PLUS_EXPR) - { - /* If the offset is compile-time constant, use it to compute a more - accurate estimate of the size of the buffer. Since the operand - of POINTER_PLUS_EXPR is represented as an unsigned type, convert - it to signed first. - Otherwise, use the size of the entire array as an optimistic - estimate (this may lead to false negatives). */ - tree adj = TREE_OPERAND (oper, 1); - adj = fold_for_warn (adj); - if (CONSTANT_CLASS_P (adj)) - adjust += wi::to_offset (convert (ssizetype, adj)); - else - use_obj_size = true; - - oper = TREE_OPERAND (oper, 0); - - STRIP_NOPS (oper); - } - - if (TREE_CODE (oper) == TARGET_EXPR) - oper = TREE_OPERAND (oper, 1); - else if (TREE_CODE (oper) == ADDR_EXPR) - { - addr_expr = true; - oper = TREE_OPERAND (oper, 0); - } - - STRIP_NOPS (oper); + access_ref ref; + ref.eval = [](tree x){ return fold_non_dependent_expr (x); }; + ref.trail1special = warn_placement_new < 2; + tree objsize = compute_objsize (oper, 1, &ref); + if (!objsize) + return; - if (TREE_CODE (oper) == ARRAY_REF - && (addr_expr || TREE_CODE (TREE_TYPE (oper)) == ARRAY_TYPE)) - { - /* Similar to the offset computed above, see if the array index - is a compile-time constant. If so, and unless the offset was - not a compile-time constant, use the index to determine the - size of the buffer. Otherwise, use the entire array as - an optimistic estimate of the size. */ - const_tree adj = fold_non_dependent_expr (TREE_OPERAND (oper, 1)); - if (!use_obj_size && CONSTANT_CLASS_P (adj)) - adjust += wi::to_offset (adj); - else - { - use_obj_size = true; - adjust = 0; - } + const bool exact_size = ref.offrng[0] == ref.offrng[1]; - oper = TREE_OPERAND (oper, 0); - } + offset_int bytes_avail = wi::to_offset (objsize); + offset_int bytes_need; - /* Refers to the declared object that constains the subobject referenced - by OPER. When the object is initialized, makes it possible to determine - the actual size of a flexible array member used as the buffer passed - as OPER to placement new. */ - tree var_decl = NULL_TREE; - /* True when operand is a COMPONENT_REF, to distinguish flexible array - members from arrays of unspecified size. */ - bool compref = TREE_CODE (oper) == COMPONENT_REF; - - /* For COMPONENT_REF (i.e., a struct member) the size of the entire - enclosing struct. Used to validate the adjustment (offset) into - an array at the end of a struct. */ - offset_int compsize = 0; - - /* Descend into a struct or union to find the member whose address - is being used as the argument. */ - if (TREE_CODE (oper) == COMPONENT_REF) + if (CONSTANT_CLASS_P (size)) + bytes_need = wi::to_offset (size); + else if (nelts && CONSTANT_CLASS_P (nelts)) + bytes_need = (wi::to_offset (nelts) + * wi::to_offset (TYPE_SIZE_UNIT (type))); + else if (tree_fits_uhwi_p (TYPE_SIZE_UNIT (type))) + bytes_need = wi::to_offset (TYPE_SIZE_UNIT (type)); + else { - tree comptype = TREE_TYPE (TREE_OPERAND (oper, 0)); - compsize = wi::to_offset (TYPE_SIZE_UNIT (comptype)); - - tree op0 = oper; - while (TREE_CODE (op0 = TREE_OPERAND (op0, 0)) == COMPONENT_REF); - STRIP_ANY_LOCATION_WRAPPER (op0); - if (VAR_P (op0)) - var_decl = op0; - oper = TREE_OPERAND (oper, 1); + /* The type is a VLA. */ + return; } - STRIP_ANY_LOCATION_WRAPPER (oper); - tree opertype = TREE_TYPE (oper); - if ((addr_expr || !INDIRECT_TYPE_P (opertype)) - && (VAR_P (oper) - || TREE_CODE (oper) == FIELD_DECL - || TREE_CODE (oper) == PARM_DECL)) - { - /* A possibly optimistic estimate of the number of bytes available - in the destination buffer. */ - offset_int bytes_avail = 0; - /* True when the estimate above is in fact the exact size - of the destination buffer rather than an estimate. */ - bool exact_size = true; - - /* Treat members of unions and members of structs uniformly, even - though the size of a member of a union may be viewed as extending - to the end of the union itself (it is by __builtin_object_size). */ - if ((VAR_P (oper) || use_obj_size) - && DECL_SIZE_UNIT (oper) - && tree_fits_uhwi_p (DECL_SIZE_UNIT (oper))) - { - /* Use the size of the entire array object when the expression - refers to a variable or its size depends on an expression - that's not a compile-time constant. */ - bytes_avail = wi::to_offset (DECL_SIZE_UNIT (oper)); - exact_size = !use_obj_size; - } - else if (tree opersize = TYPE_SIZE_UNIT (opertype)) - { - /* Use the size of the type of the destination buffer object - as the optimistic estimate of the available space in it. - Use the maximum possible size for zero-size arrays and - flexible array members (except of initialized objects - thereof). */ - if (TREE_CODE (opersize) == INTEGER_CST) - bytes_avail = wi::to_offset (opersize); - } - - if (bytes_avail == 0) - { - if (var_decl) - { - /* Constructing into a buffer provided by the flexible array - member of a declared object (which is permitted as a G++ - extension). If the array member has been initialized, - determine its size from the initializer. Otherwise, - the array size is zero. */ - if (tree init = find_flexarray_init (oper, - DECL_INITIAL (var_decl))) - bytes_avail = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (init))); - } - else - bytes_avail = (wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node)) - - compsize); - } - - tree_code oper_code = TREE_CODE (opertype); - - if (compref && oper_code == ARRAY_TYPE) - { - tree nelts = array_type_nelts_top (opertype); - tree nelts_cst = maybe_constant_value (nelts); - if (TREE_CODE (nelts_cst) == INTEGER_CST - && integer_onep (nelts_cst) - && !var_decl - && warn_placement_new < 2) - return; - } - - /* Reduce the size of the buffer by the adjustment computed above - from the offset and/or the index into the array. */ - if (bytes_avail < adjust || adjust < 0) - bytes_avail = 0; - else - { - tree elttype = (TREE_CODE (opertype) == ARRAY_TYPE - ? TREE_TYPE (opertype) : opertype); - if (tree eltsize = TYPE_SIZE_UNIT (elttype)) - { - bytes_avail -= adjust * wi::to_offset (eltsize); - if (bytes_avail < 0) - bytes_avail = 0; - } - } - - /* The minimum amount of space needed for the allocation. This - is an optimistic estimate that makes it possible to detect - placement new invocation for some undersize buffers but not - others. */ - offset_int bytes_need; + if (bytes_avail >= bytes_need) + return; - if (nelts) - nelts = fold_for_warn (nelts); - - if (CONSTANT_CLASS_P (size)) - bytes_need = wi::to_offset (size); - else if (nelts && CONSTANT_CLASS_P (nelts)) - bytes_need = (wi::to_offset (nelts) - * wi::to_offset (TYPE_SIZE_UNIT (type))); - else if (tree_fits_uhwi_p (TYPE_SIZE_UNIT (type))) - bytes_need = wi::to_offset (TYPE_SIZE_UNIT (type)); - else - { - /* The type is a VLA. */ - return; - } + tree opertype = ref.ref ? TREE_TYPE (ref.ref) : TREE_TYPE (oper); + bool warned = false; + if (nelts) + nelts = fold_for_warn (nelts); + if (nelts) + if (CONSTANT_CLASS_P (nelts)) + warned = warning_at (loc, OPT_Wplacement_new_, + (exact_size + ? G_("placement new constructing an object " + "of type %<%T [%wu]%> and size %qwu " + "in a region of type %qT and size %qwi") + : G_("placement new constructing an object " + "of type %<%T [%wu]%> and size %qwu " + "in a region of type %qT and size " + "at most %qwu")), + type, tree_to_uhwi (nelts), + bytes_need.to_uhwi (), + opertype, bytes_avail.to_uhwi ()); + else + warned = warning_at (loc, OPT_Wplacement_new_, + (exact_size + ? G_("placement new constructing an array " + "of objects of type %qT and size %qwu " + "in a region of type %qT and size %qwi") + : G_("placement new constructing an array " + "of objects of type %qT and size %qwu " + "in a region of type %qT and size " + "at most %qwu")), + type, bytes_need.to_uhwi (), opertype, + bytes_avail.to_uhwi ()); + else + warned = warning_at (loc, OPT_Wplacement_new_, + (exact_size + ? G_("placement new constructing an object " + "of type %qT and size %qwu in a region " + "of type %qT and size %qwi") + : G_("placement new constructing an object " + "of type %qT " + "and size %qwu in a region of type %qT " + "and size at most %qwu")), + type, bytes_need.to_uhwi (), opertype, + bytes_avail.to_uhwi ()); + + if (!warned || !ref.ref) + return; - if (bytes_avail < bytes_need) - { - if (nelts) - if (CONSTANT_CLASS_P (nelts)) - warning_at (loc, OPT_Wplacement_new_, - exact_size ? - "placement new constructing an object of type " - "%<%T [%wu]%> and size %qwu in a region of type %qT " - "and size %qwi" - : "placement new constructing an object of type " - "%<%T [%wu]%> and size %qwu in a region of type %qT " - "and size at most %qwu", - type, tree_to_uhwi (nelts), bytes_need.to_uhwi (), - opertype, bytes_avail.to_uhwi ()); - else - warning_at (loc, OPT_Wplacement_new_, - exact_size ? - "placement new constructing an array of objects " - "of type %qT and size %qwu in a region of type %qT " - "and size %qwi" - : "placement new constructing an array of objects " - "of type %qT and size %qwu in a region of type %qT " - "and size at most %qwu", - type, bytes_need.to_uhwi (), opertype, - bytes_avail.to_uhwi ()); - else - warning_at (loc, OPT_Wplacement_new_, - exact_size ? - "placement new constructing an object of type %qT " - "and size %qwu in a region of type %qT and size %qwi" - : "placement new constructing an object of type %qT " - "and size %qwu in a region of type %qT and size " - "at most %qwu", - type, bytes_need.to_uhwi (), opertype, - bytes_avail.to_uhwi ()); - } - } + 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); } /* True if alignof(T) > __STDCPP_DEFAULT_NEW_ALIGNMENT__. */ diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc index c2dd6663c3a..fd4eb443f3e 100644 --- a/gcc/gimple-array-bounds.cc +++ b/gcc/gimple-array-bounds.cc @@ -68,7 +68,7 @@ array_bounds_checker::check_array_ref (location_t location, tree ref, tree decl = NULL_TREE; /* Set for accesses to interior zero-length arrays. */ - bool interior_zero_len = false; + special_array_member sam{ }; tree up_bound_p1; @@ -101,7 +101,7 @@ array_bounds_checker::check_array_ref (location_t location, tree ref, { /* Try to determine the size of the trailing array from its initializer (if it has one). */ - if (tree refsize = component_ref_size (arg, &interior_zero_len)) + if (tree refsize = component_ref_size (arg, &sam)) if (TREE_CODE (refsize) == INTEGER_CST) maxbound = refsize; } @@ -199,7 +199,7 @@ array_bounds_checker::check_array_ref (location_t location, tree ref, "array subscript %E is below array bounds of %qT", low_sub, artype); - if (!warned && interior_zero_len) + if (!warned && sam == special_array_member::int_0) warned = warning_at (location, OPT_Wzero_length_bounds, (TREE_CODE (low_sub) == INTEGER_CST ? G_("array subscript %E is outside the bounds " diff --git a/gcc/testsuite/g++.dg/init/strlen.C b/gcc/testsuite/g++.dg/init/strlen.C index aa8950e2dc0..cc650d65dbe 100644 --- a/gcc/testsuite/g++.dg/init/strlen.C +++ b/gcc/testsuite/g++.dg/init/strlen.C @@ -29,7 +29,7 @@ test_dynamic_type (S *p) // distinguish invalid cases from ones like it that might be valid. // If/when GIMPLE changes to make this possible this test can be // removed. - char *q = new (p->a) char [16]; + char *q = new (p->a) char [16]; // { dg-warning "\\\[-Wplacement-new" } init (q); diff --git a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-1.C b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-1.C index d2ec608afd4..cec83163dbe 100644 --- a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-1.C +++ b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-1.C @@ -66,8 +66,9 @@ struct BA2 { int i; A2 a2; }; void fBx (BAx *pbx, BAx &rbx) { BAx bax; - new (bax.ax.a) char; // { dg-warning "placement" } - new (bax.ax.a) Int16; // { dg-warning "placement" } + // The uninitialized flexible array takes up the bytes of padding. + new (bax.ax.a) char; + new (bax.ax.a) Int16; new (bax.ax.a) Int32; // { dg-warning "placement" } new (pbx->ax.a) char; @@ -84,9 +85,12 @@ void fBx1 () { static BAx bax1 = { 1, /* Ax = */ { 2, /* a[] = */ {} } }; - new (bax1.ax.a) char; // { dg-warning "placement" } - new (bax1.ax.a) char[2]; // { dg-warning "placement" } - new (bax1.ax.a) Int16; // { dg-warning "placement" } + // The empty flexible array takes up the bytes of padding. + new (bax1.ax.a) char; + new (bax1.ax.a) char[2]; + new (bax1.ax.a) Int16; + new (bax1.ax.a) char[3]; + new (bax1.ax.a) char[4]; // { dg-warning "placement" } new (bax1.ax.a) Int32; // { dg-warning "placement" } } diff --git a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-2.C b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-2.C index e00515eeaa9..e5fdfe1f603 100644 --- a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-2.C +++ b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-2.C @@ -124,9 +124,13 @@ struct BA2 { int i; A2 a2; }; void fBx (BAx *pbx, BAx &rbx) { BAx bax; - new (bax.ax.a) char; // { dg-warning "placement" } - new (bax.ax.a) Int16; // { dg-warning "placement" } + // The uninitialized flexible array takes up the bytes of padding. + new (bax.ax.a) char; + new (bax.ax.a) Int16; + new (bax.ax.a) char[3]; new (bax.ax.a) Int32; // { dg-warning "placement" } + new (bax.ax.a) char[4]; // { dg-warning "placement" } + new (bax.ax.a) char[5]; // { dg-warning "placement" } new (pbx->ax.a) char; new (rbx.ax.a) char; @@ -142,10 +146,14 @@ void fBx1 () { static BAx bax1 = { 1, /* Ax = */ { 2, /* a[] = */ {} } }; - new (bax1.ax.a) char; // { dg-warning "placement" } - new (bax1.ax.a) char[2]; // { dg-warning "placement" } - new (bax1.ax.a) Int16; // { dg-warning "placement" } + // The empty flexible array takes up the bytes of padding. + new (bax1.ax.a) char; + new (bax1.ax.a) char[2]; + new (bax1.ax.a) Int16; + new (bax1.ax.a) char[3]; new (bax1.ax.a) Int32; // { dg-warning "placement" } + new (bax1.ax.a) char[4]; // { dg-warning "placement" } + new (bax1.ax.a) char[5]; // { dg-warning "placement" } } void fB0 (BA0 *pb0, BA0 &rb0) diff --git a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-6.C b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-6.C index b6a72b18f6a..5eb63d23b47 100644 --- a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-6.C +++ b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-6.C @@ -17,9 +17,10 @@ void fBx1 () { static BAx bax1 = { 1, /* Ax = */ { 2, /* a[] = */ { 3 } } }; // { dg-error "initialization of flexible array member in a nested context" } - new (bax1.ax.a) char; // { dg-warning "placement" } - new (bax1.ax.a) char[2]; // { dg-warning "placement" } - new (bax1.ax.a) Int16; // { dg-warning "placement" } + // The first three bytes of the flexible array member live in the padding. + new (bax1.ax.a) char; + new (bax1.ax.a) char[2]; + new (bax1.ax.a) Int16; new (bax1.ax.a) Int32; // { dg-warning "placement" } } @@ -27,10 +28,11 @@ void fBx2 () { static BAx bax2 = { 1, /* Ax = */ { 2, /* a[] = */ { 3, 4 } } }; // { dg-error "initialization of flexible array member in a nested context" } - new (bax2.ax.a) char; // { dg-warning "placement" } - new (bax2.ax.a) char[2]; // { dg-warning "placement" } - new (bax2.ax.a) char[3]; // { dg-warning "placement" } - new (bax2.ax.a) Int16; // { dg-warning "placement" } + // The first three bytes of the flexible array member live in the padding. + new (bax2.ax.a) char; + new (bax2.ax.a) char[2]; + new (bax2.ax.a) char[3]; + new (bax2.ax.a) Int16; new (bax2.ax.a) char[4]; // { dg-warning "placement" } new (bax2.ax.a) Int32; // { dg-warning "placement" } } @@ -39,10 +41,11 @@ void fBx3 () { static BAx bax2 = { 1, /* Ax = */ { 3, /* a[] = */ { 4, 5, 6 } } }; // { dg-error "initialization of flexible array member in a nested context" } - new (bax2.ax.a) char; // { dg-warning "placement" } - new (bax2.ax.a) char[2]; // { dg-warning "placement" } - new (bax2.ax.a) Int16; // { dg-warning "placement" } - new (bax2.ax.a) char[3]; // { dg-warning "placement" } + // The first three bytes of the flexible array member live in the padding. + new (bax2.ax.a) char; + new (bax2.ax.a) char[2]; + new (bax2.ax.a) Int16; + new (bax2.ax.a) char[3]; new (bax2.ax.a) char[4]; // { dg-warning "placement" } new (bax2.ax.a) Int32; // { dg-warning "placement" } } diff --git a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-7.C b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-7.C new file mode 100644 index 00000000000..82f298d8008 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-7.C @@ -0,0 +1,82 @@ +/* PR c++/96511 - Incorrect -Wplacement-new on POINTER_PLUS into an array + with 4-byte elements + { dg-do compile } + { dg-options "-Wall" } */ + +typedef __INT16_TYPE__ int16_t; +typedef __INT32_TYPE__ int32_t; +typedef __SIZE_TYPE__ size_t; + +void* operator new (size_t, void *p) { return p; } + +void test_a1_int16 () +{ + int16_t a3[3]; // { dg-message "declared here" } + + new (a3) int16_t; + new (a3 + 1) int16_t; + new (a3 + 2) int16_t; // { dg-bogus "\\\[-Wplacement-new" } + new (&a3[1]) int16_t; + new (&a3[0] + 1) int16_t; + new (&a3[0] + 2) int16_t; // { dg-bogus "\\\[-Wplacement-new" } + new (&a3[0] + 3) int16_t; // { dg-warning "\\\[-Wplacement-new" } +} + +void test_a1_int32 () +{ + int16_t a3[3]; + + new (a3 + 1) int32_t; // { dg-bogus "\\\[-Wplacement-new" } + new (&a3[1]) int32_t; + new (&a3[0] + 1) int32_t; // { dg-bogus "\\\[-Wplacement-new" } + new (&a3[0] + 2) int32_t; // { dg-warning "\\\[-Wplacement-new" } +} + + +void test_a2 () +{ + int16_t a23[2][3]; + + new (a23 + 1) int16_t; // { dg-bogus "\\\[-Wplacement-new" } + new (&a23[1]) int16_t; + new (&a23[2]) int16_t; // { dg-warning "\\\[-Wplacement-new" } + + new (&a23[0][0] + 1) int16_t; + new (&a23[0][0] + 2) int16_t; + // Deriving a pointer to the next array from one to an element of + // the prior array isn't valid even if the resulting pointer points + // to an element of the larger array. Verify it's diagnosed. + new (&a23[0][0] + 3) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][0] + 4) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][0] + 5) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][0] + 6) int16_t; // { dg-warning "\\\[-Wplacement-new" } + + new (&a23[0][1] + 1) int16_t; + new (&a23[0][1] + 2) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][1] + 3) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][1] + 4) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][1] + 5) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][1] + 6) int16_t; // { dg-warning "\\\[-Wplacement-new" } + + new (&a23[0][2] + 1) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][2] + 2) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][2] + 3) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][2] + 4) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][2] + 5) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[0][2] + 6) int16_t; // { dg-warning "\\\[-Wplacement-new" } + + new (&a23[1][0]) int16_t; + new (&a23[1][0] + 1) int16_t; + new (&a23[1][0] + 2) int16_t; + new (&a23[1][0] + 3) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[1][0] + 4) int16_t; // { dg-warning "\\\[-Wplacement-new" } + + new (&a23[1][1]) int16_t; + new (&a23[1][2]) int16_t; + new (&a23[1][2] + 1) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[1][3]) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[1][3] + 1) int16_t; // { dg-warning "\\\[-Wplacement-new" } + + new (&a23[2][0]) int16_t; // { dg-warning "\\\[-Wplacement-new" } + new (&a23[2][0] + 1) int16_t; // { dg-warning "\\\[-Wplacement-new" } +} diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c new file mode 100644 index 00000000000..cd8fa3202eb --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c @@ -0,0 +1,116 @@ +/* PR middle-end/96384 - bogus -Wstringop-overflow= storing into + multidimensional array with index in range + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +#define SHRT_MAX __SHRT_MAX__ +#define SHRT_MIN (-SHRT_MAX - 1) +#define INT_MAX __INT_MAX__ +#define INT_MIN (-INT_MAX - 1) +#define LONG_MAX __LONG_MAX__ +#define LONG_MIN (-LONG_MAX - 1) + +#define USHRT_MAX (SHRT_MAX * 2 + 1) +#define UINT_MAX ~0U +#define ULONG_MAX ~0LU + +char ca3_5_7[3][5][7]; + +void nowarn_ca_3_5_ssi (short i) +{ + if (i > SHRT_MAX - 1) + i = SHRT_MAX - 1; + + ca3_5_7[i][0][0] = __LINE__; + ca3_5_7[i][0][1] = __LINE__; + ca3_5_7[i][0][2] = __LINE__; + ca3_5_7[i][0][3] = __LINE__; + ca3_5_7[i][0][4] = __LINE__; + ca3_5_7[i][0][5] = __LINE__; + ca3_5_7[i][0][6] = __LINE__; + + ca3_5_7[i][1][0] = __LINE__; + ca3_5_7[i][1][1] = __LINE__; + ca3_5_7[i][1][2] = __LINE__; + ca3_5_7[i][1][3] = __LINE__; + ca3_5_7[i][1][4] = __LINE__; + ca3_5_7[i][1][5] = __LINE__; + ca3_5_7[i][1][6] = __LINE__; + + ca3_5_7[i][2][0] = __LINE__; + ca3_5_7[i][2][1] = __LINE__; + ca3_5_7[i][2][2] = __LINE__; + ca3_5_7[i][2][3] = __LINE__; + ca3_5_7[i][2][4] = __LINE__; + ca3_5_7[i][2][5] = __LINE__; + ca3_5_7[i][2][6] = __LINE__; + + ca3_5_7[i][3][0] = __LINE__; + ca3_5_7[i][3][1] = __LINE__; + ca3_5_7[i][3][2] = __LINE__; + ca3_5_7[i][3][3] = __LINE__; + ca3_5_7[i][3][4] = __LINE__; + ca3_5_7[i][3][5] = __LINE__; + ca3_5_7[i][3][6] = __LINE__; + + ca3_5_7[i][4][0] = __LINE__; + ca3_5_7[i][4][1] = __LINE__; + ca3_5_7[i][4][2] = __LINE__; + ca3_5_7[i][4][3] = __LINE__; + ca3_5_7[i][4][4] = __LINE__; + ca3_5_7[i][4][5] = __LINE__; + ca3_5_7[i][4][6] = __LINE__; + + ca3_5_7[1][i][5] = __LINE__; + ca3_5_7[2][3][i] = __LINE__; +} + +void nowarn_ca_3_5_usi (unsigned short i) +{ + if (i > USHRT_MAX - 1) + i = USHRT_MAX - 1; + + ca3_5_7[i][3][5] = __LINE__; + ca3_5_7[1][i][5] = __LINE__; + ca3_5_7[2][3][i] = __LINE__; +} + +void nowarn_ca_3_5_si (int i) +{ + if (i > INT_MAX - 1) + i = INT_MAX - 1; + + ca3_5_7[i][3][5] = __LINE__; + ca3_5_7[1][i][5] = __LINE__; + ca3_5_7[2][3][i] = __LINE__; +} + +void nowarn_ca_3_5_ui (unsigned i) +{ + if (i > UINT_MAX - 1) + i = UINT_MAX - 1; + + ca3_5_7[i][3][5] = __LINE__; + ca3_5_7[1][i][5] = __LINE__; + ca3_5_7[2][3][i] = __LINE__; +} + +void nowarn_ca_3_5_li (long i) +{ + if (i > LONG_MAX - 1) + i = LONG_MAX - 1; + + ca3_5_7[i][3][5] = __LINE__; + ca3_5_7[1][i][5] = __LINE__; + ca3_5_7[2][3][i] = __LINE__; +} + +void nowarn_ca_3_5_uli (unsigned long i) +{ + if (i > ULONG_MAX - 1) + i = ULONG_MAX - 1; + + ca3_5_7[i][3][5] = __LINE__; + ca3_5_7[1][i][5] = __LINE__; + ca3_5_7[2][3][i] = __LINE__; +} diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-21.c b/gcc/testsuite/gcc.dg/builtin-object-size-21.c index 1c42374ba89..a232b500a51 100644 --- a/gcc/testsuite/gcc.dg/builtin-object-size-21.c +++ b/gcc/testsuite/gcc.dg/builtin-object-size-21.c @@ -1,7 +1,7 @@ /* PR middle-end/92815 - spurious -Wstringop-overflow writing into a flexible array of an extern struct { dg-do compile } - { dg-options "-Wall -fdump-tree-optimized" } */ + { dg-options "-Wall -fdump-tree-optimized -o/dev/null" } */ #define PTRDIFF_MAX __PTRDIFF_MAX__ diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c index bc55b27cff0..38632a51ac9 100644 --- a/gcc/tree-object-size.c +++ b/gcc/tree-object-size.c @@ -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); /* Use TYPE_SIZE_UNIT; DECL_SIZE_UNIT sometimes reflects the size of the initializer and sometimes doesn't. */ diff --git a/gcc/tree.c b/gcc/tree.c index 6dea32aeb45..af3d12ccaeb 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -13623,20 +13623,21 @@ get_initializer_for (tree init, tree decl) /* Determines the size of the member referenced by the COMPONENT_REF REF, using its initializer expression if necessary in order to determine the size of an initialized flexible array member. - If non-null, *INTERIOR_ZERO_LENGTH is set when REF refers to - an interior zero-length array. + If non-null, set *ARK when REF refers to an interior zero-length + array or a trailing one-element array. Returns the size as sizetype (which might be zero for an object with an uninitialized flexible array member) or null if the size cannot be determined. */ tree -component_ref_size (tree ref, bool *interior_zero_length /* = NULL */) +component_ref_size (tree ref, special_array_member *sam /* = NULL */) { gcc_assert (TREE_CODE (ref) == COMPONENT_REF); - bool int_0_len = false; - if (!interior_zero_length) - interior_zero_length = &int_0_len; + special_array_member arkbuf; + if (!sam) + sam = &arkbuf; + *sam = special_array_member::none; /* The object/argument referenced by the COMPONENT_REF and its type. */ tree arg = TREE_OPERAND (ref, 0); @@ -13658,9 +13659,16 @@ component_ref_size (tree ref, bool *interior_zero_length /* = NULL */) more than one element. */ return memsize; - *interior_zero_length = zero_length && !trailing; - if (*interior_zero_length) - memsize = NULL_TREE; + if (zero_length) + { + if (trailing) + *sam = special_array_member::trail_0; + else + { + *sam = special_array_member::int_0; + memsize = NULL_TREE; + } + } if (!zero_length) if (tree dom = TYPE_DOMAIN (memtype)) @@ -13671,9 +13679,13 @@ component_ref_size (tree ref, bool *interior_zero_length /* = NULL */) { offset_int minidx = wi::to_offset (min); offset_int maxidx = wi::to_offset (max); - if (maxidx - minidx > 0) + offset_int neltsm1 = maxidx - minidx; + if (neltsm1 > 0) /* MEMBER is an array with more than one element. */ return memsize; + + if (neltsm1 == 0) + *sam = special_array_member::trail_1; } /* For a refernce to a zero- or one-element array member of a union @@ -13691,7 +13703,7 @@ component_ref_size (tree ref, bool *interior_zero_length /* = NULL */) tree base = get_addr_base_and_unit_offset (ref, &baseoff); if (!base || !VAR_P (base)) { - if (!*interior_zero_length) + if (*sam != special_array_member::int_0) return NULL_TREE; if (TREE_CODE (arg) != COMPONENT_REF) @@ -13711,7 +13723,7 @@ component_ref_size (tree ref, bool *interior_zero_length /* = NULL */) /* Determine the base type of the referenced object. If it's the same as ARGTYPE and MEMBER has a known size, return it. */ tree bt = basetype; - if (!*interior_zero_length) + if (*sam != special_array_member::int_0) while (TREE_CODE (bt) == ARRAY_TYPE) bt = TREE_TYPE (bt); bool typematch = useless_type_conversion_p (argtype, bt); @@ -13751,7 +13763,7 @@ component_ref_size (tree ref, bool *interior_zero_length /* = NULL */) if (DECL_P (base) && DECL_EXTERNAL (base) && bt == basetype - && !*interior_zero_length) + && *sam != special_array_member::int_0) /* The size of a flexible array member of an extern struct with no initializer cannot be determined (it's defined in another translation unit and can have an initializer diff --git a/gcc/tree.h b/gcc/tree.h index 22dd4ac0f3c..7e289c22678 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -5283,12 +5283,22 @@ extern bool array_at_struct_end_p (tree); by EXP. This does not include any offset in DECL_FIELD_BIT_OFFSET. */ extern tree component_ref_field_offset (tree); +/* Describes a "special" array member due to which component_ref_size + returns null. */ +enum struct special_array_member + { + none, /* Not a special array member. */ + int_0, /* Interior array member with size zero. */ + trail_0, /* Trailing array member with size zero. */ + trail_1 /* Trailing array member with one element. */ + }; + /* Return the size of the member referenced by the COMPONENT_REF, using its initializer expression if necessary in order to determine the size of an initialized flexible array member. The size might be zero for an object with an uninitialized flexible array member or null if it cannot be determined. */ -extern tree component_ref_size (tree, bool * = NULL); +extern tree component_ref_size (tree, special_array_member * = NULL); extern int tree_map_base_eq (const void *, const void *); extern unsigned int tree_map_base_hash (const void *);