| Message ID | yddmvgen359.fsf@CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
On 12/02/2016 01:31 AM, Rainer Orth wrote: > Hi Martin, > >> PR 78521 notes that the gimple-ssa-sprintf pass doesn't do the right >> thing (i.e., the -Wformat-length and -fprintf-return-value options >> behave incorrectly) when a conversion specification includes a width >> or precision with a non-constant value. The code treats such cases >> as if they were not provided which is incorrect and results in >> the wrong bytes counts in warning messages and in the wrong ranges >> being generated for such calls (or in the case sprintf(0, 0, ...) >> for some such calls being eliminated). >> >> The attached patch corrects the handling of these cases, plus a couple >> of other edge cases in the same area: it adjusts the parser to accept >> precision in the form of just a period with no asterisk or decimal >> digits after it (this sets the precision to zero), and corrects the >> handling of zero precision and zero argument in integer directives >> to produce no bytes on output. >> >> Finally, the patch also tightens up the constraint on the upper bound >> of bounded functions like snprintf to be INT_MAX. The functions cannot >> produce output in excess of INT_MAX + 1 bytes and some implementations >> (e.g., Solaris) fail with EINVAL when the bound is INT_MAX or more. >> This is the subject of PR 78520. > > this patch broke Solaris bootstrap: > > /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 'void {anonymous}::get_width_and_precision(const {anonymous}::conversion_spec&, long long int*, long long int*)': > /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: error: call of overloaded 'abs(long long int)' is ambiguous > width = abs (tree_to_shwi (spec.star_width)); > ^ > /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: note: candidates are: > In file included from /usr/include/stdlib.h:12:0, > from /vol/gcc/src/hg/trunk/local/gcc/system.h:258, > from /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:49: > /usr/include/iso/stdlib_iso.h:205:16: note: long int std::abs(long int) > inline long abs(long _l) { return labs(_l); } > ^ > /usr/include/iso/stdlib_iso.h:160:12: note: int std::abs(int) > extern int abs(int); > ^ > > The following patch fixed this for me, but I've no idea if it's right. > It bootstrapped successfully on sparc-sun-solaris2.12, > i386-pc-solaris2.12, and x86_64-pc-linux-gnu. Thanks for the heads up! I just looked at that code yesterday while analyzing bug 78608, wondering if it was safe. Now I know it isn't. I think it might be best to simply hand code the expression instead of taking a chance on abs. Let me take care of it today along with 78608. Martin
On 12/02/2016 08:52 AM, Martin Sebor wrote: > On 12/02/2016 01:31 AM, Rainer Orth wrote: >> Hi Martin, >> >>> PR 78521 notes that the gimple-ssa-sprintf pass doesn't do the right >>> thing (i.e., the -Wformat-length and -fprintf-return-value options >>> behave incorrectly) when a conversion specification includes a width >>> or precision with a non-constant value. The code treats such cases >>> as if they were not provided which is incorrect and results in >>> the wrong bytes counts in warning messages and in the wrong ranges >>> being generated for such calls (or in the case sprintf(0, 0, ...) >>> for some such calls being eliminated). >>> >>> The attached patch corrects the handling of these cases, plus a couple >>> of other edge cases in the same area: it adjusts the parser to accept >>> precision in the form of just a period with no asterisk or decimal >>> digits after it (this sets the precision to zero), and corrects the >>> handling of zero precision and zero argument in integer directives >>> to produce no bytes on output. >>> >>> Finally, the patch also tightens up the constraint on the upper bound >>> of bounded functions like snprintf to be INT_MAX. The functions cannot >>> produce output in excess of INT_MAX + 1 bytes and some implementations >>> (e.g., Solaris) fail with EINVAL when the bound is INT_MAX or more. >>> This is the subject of PR 78520. >> >> this patch broke Solaris bootstrap: >> >> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function >> 'void {anonymous}::get_width_and_precision(const >> {anonymous}::conversion_spec&, long long int*, long long int*)': >> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: error: >> call of overloaded 'abs(long long int)' is ambiguous >> width = abs (tree_to_shwi (spec.star_width)); >> ^ >> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: note: >> candidates are: >> In file included from /usr/include/stdlib.h:12:0, >> from /vol/gcc/src/hg/trunk/local/gcc/system.h:258, >> from >> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:49: >> /usr/include/iso/stdlib_iso.h:205:16: note: long int std::abs(long int) >> inline long abs(long _l) { return labs(_l); } >> ^ >> /usr/include/iso/stdlib_iso.h:160:12: note: int std::abs(int) >> extern int abs(int); >> ^ >> >> The following patch fixed this for me, but I've no idea if it's right. >> It bootstrapped successfully on sparc-sun-solaris2.12, >> i386-pc-solaris2.12, and x86_64-pc-linux-gnu. > > Thanks for the heads up! I just looked at that code yesterday while > analyzing bug 78608, wondering if it was safe. Now I know it isn't. > I think it might be best to simply hand code the expression instead > of taking a chance on abs. Let me take care of it today along with 78608. I posted a bigger patch to fix this and other related problems on Friday (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html). In hindsight, I should have probably committed the fix for this on its own. Please let me know if this is blocking you and I'll commit this fix by itself today so you don't have to wait for the bigger patch to get reviewed and approved. Martin
On Mon, Dec 05, 2016 at 08:50:08AM -0700, Martin Sebor wrote: > I posted a bigger patch to fix this and other related problems on > Friday (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html). > In hindsight, I should have probably committed the fix for this > on its own. Please let me know if this is blocking you and I'll > commit this fix by itself today so you don't have to wait for > the bigger patch to get reviewed and approved. You could just change the abs use to absu_hwi or abs_hwi if you need something quickly working (depending on whether HOST_WIDE_INT_MIN can appear or not). Jakub
On 12/05/2016 08:50 AM, Martin Sebor wrote: > On 12/02/2016 08:52 AM, Martin Sebor wrote: >> On 12/02/2016 01:31 AM, Rainer Orth wrote: >>> Hi Martin, >>> >>>> PR 78521 notes that the gimple-ssa-sprintf pass doesn't do the right >>>> thing (i.e., the -Wformat-length and -fprintf-return-value options >>>> behave incorrectly) when a conversion specification includes a width >>>> or precision with a non-constant value. The code treats such cases >>>> as if they were not provided which is incorrect and results in >>>> the wrong bytes counts in warning messages and in the wrong ranges >>>> being generated for such calls (or in the case sprintf(0, 0, ...) >>>> for some such calls being eliminated). >>>> >>>> The attached patch corrects the handling of these cases, plus a couple >>>> of other edge cases in the same area: it adjusts the parser to accept >>>> precision in the form of just a period with no asterisk or decimal >>>> digits after it (this sets the precision to zero), and corrects the >>>> handling of zero precision and zero argument in integer directives >>>> to produce no bytes on output. >>>> >>>> Finally, the patch also tightens up the constraint on the upper bound >>>> of bounded functions like snprintf to be INT_MAX. The functions cannot >>>> produce output in excess of INT_MAX + 1 bytes and some implementations >>>> (e.g., Solaris) fail with EINVAL when the bound is INT_MAX or more. >>>> This is the subject of PR 78520. >>> >>> this patch broke Solaris bootstrap: >>> >>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function >>> 'void {anonymous}::get_width_and_precision(const >>> {anonymous}::conversion_spec&, long long int*, long long int*)': >>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: error: >>> call of overloaded 'abs(long long int)' is ambiguous >>> width = abs (tree_to_shwi (spec.star_width)); >>> ^ >>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: note: >>> candidates are: >>> In file included from /usr/include/stdlib.h:12:0, >>> from /vol/gcc/src/hg/trunk/local/gcc/system.h:258, >>> from >>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:49: >>> /usr/include/iso/stdlib_iso.h:205:16: note: long int std::abs(long int) >>> inline long abs(long _l) { return labs(_l); } >>> ^ >>> /usr/include/iso/stdlib_iso.h:160:12: note: int std::abs(int) >>> extern int abs(int); >>> ^ >>> >>> The following patch fixed this for me, but I've no idea if it's right. >>> It bootstrapped successfully on sparc-sun-solaris2.12, >>> i386-pc-solaris2.12, and x86_64-pc-linux-gnu. >> >> Thanks for the heads up! I just looked at that code yesterday while >> analyzing bug 78608, wondering if it was safe. Now I know it isn't. >> I think it might be best to simply hand code the expression instead >> of taking a chance on abs. Let me take care of it today along with >> 78608. > > I posted a bigger patch to fix this and other related problems on > Friday (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html). > In hindsight, I should have probably committed the fix for this > on its own. Please let me know if this is blocking you and I'll > commit this fix by itself today so you don't have to wait for > the bigger patch to get reviewed and approved. What's the concern with using std::abs? We're already using std::min std::max, std::swap and others. jeff
On Mon, Dec 05, 2016 at 11:25:02AM -0700, Jeff Law wrote:
> We're already using std::min std::max, std::swap and others.
Note we're not using std::min nor std::max. I gave this a shot a while ago,
but it didn't pan out:
https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00886.html
Marek
On Mon, Dec 05, 2016 at 11:25:02AM -0700, Jeff Law wrote: > >> > >>Thanks for the heads up! I just looked at that code yesterday while > >>analyzing bug 78608, wondering if it was safe. Now I know it isn't. > >>I think it might be best to simply hand code the expression instead > >>of taking a chance on abs. Let me take care of it today along with > >>78608. > > > >I posted a bigger patch to fix this and other related problems on > >Friday (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html). > >In hindsight, I should have probably committed the fix for this > >on its own. Please let me know if this is blocking you and I'll > >commit this fix by itself today so you don't have to wait for > >the bigger patch to get reviewed and approved. > What's the concern with using std::abs? We already have abs_hwi and absu_hwi where you choose the semantics you want. std::abs might not even have the right overload for HWI. Jakub
On 12/05/2016 11:30 AM, Marek Polacek wrote: > On Mon, Dec 05, 2016 at 11:25:02AM -0700, Jeff Law wrote: >> We're already using std::min std::max, std::swap and others. > > Note we're not using std::min nor std::max. I gave this a shot a while ago, > but it didn't pan out: > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00886.html > > Marek > tree-ssa-phiprop.c uses std::min and std::max Jeff
On Mon, Dec 05, 2016 at 11:37:23AM -0700, Jeff Law wrote: > On 12/05/2016 11:30 AM, Marek Polacek wrote: > >On Mon, Dec 05, 2016 at 11:25:02AM -0700, Jeff Law wrote: > >>We're already using std::min std::max, std::swap and others. > > > >Note we're not using std::min nor std::max. I gave this a shot a while ago, > >but it didn't pan out: > >https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00886.html > > > > Marek > > > tree-ssa-phiprop.c uses std::min and std::max If you mean the std::max(std::min(a0, c), std::min(std::max(a1, c), b)) line, that is in a comment. Jakub
On 12/05/2016 11:25 AM, Jeff Law wrote: > On 12/05/2016 08:50 AM, Martin Sebor wrote: >> On 12/02/2016 08:52 AM, Martin Sebor wrote: >>> On 12/02/2016 01:31 AM, Rainer Orth wrote: >>>> Hi Martin, >>>> >>>>> PR 78521 notes that the gimple-ssa-sprintf pass doesn't do the right >>>>> thing (i.e., the -Wformat-length and -fprintf-return-value options >>>>> behave incorrectly) when a conversion specification includes a width >>>>> or precision with a non-constant value. The code treats such cases >>>>> as if they were not provided which is incorrect and results in >>>>> the wrong bytes counts in warning messages and in the wrong ranges >>>>> being generated for such calls (or in the case sprintf(0, 0, ...) >>>>> for some such calls being eliminated). >>>>> >>>>> The attached patch corrects the handling of these cases, plus a couple >>>>> of other edge cases in the same area: it adjusts the parser to accept >>>>> precision in the form of just a period with no asterisk or decimal >>>>> digits after it (this sets the precision to zero), and corrects the >>>>> handling of zero precision and zero argument in integer directives >>>>> to produce no bytes on output. >>>>> >>>>> Finally, the patch also tightens up the constraint on the upper bound >>>>> of bounded functions like snprintf to be INT_MAX. The functions >>>>> cannot >>>>> produce output in excess of INT_MAX + 1 bytes and some implementations >>>>> (e.g., Solaris) fail with EINVAL when the bound is INT_MAX or more. >>>>> This is the subject of PR 78520. >>>> >>>> this patch broke Solaris bootstrap: >>>> >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function >>>> 'void {anonymous}::get_width_and_precision(const >>>> {anonymous}::conversion_spec&, long long int*, long long int*)': >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: error: >>>> call of overloaded 'abs(long long int)' is ambiguous >>>> width = abs (tree_to_shwi (spec.star_width)); >>>> ^ >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: note: >>>> candidates are: >>>> In file included from /usr/include/stdlib.h:12:0, >>>> from /vol/gcc/src/hg/trunk/local/gcc/system.h:258, >>>> from >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:49: >>>> /usr/include/iso/stdlib_iso.h:205:16: note: long int std::abs(long int) >>>> inline long abs(long _l) { return labs(_l); } >>>> ^ >>>> /usr/include/iso/stdlib_iso.h:160:12: note: int std::abs(int) >>>> extern int abs(int); >>>> ^ >>>> >>>> The following patch fixed this for me, but I've no idea if it's right. >>>> It bootstrapped successfully on sparc-sun-solaris2.12, >>>> i386-pc-solaris2.12, and x86_64-pc-linux-gnu. >>> >>> Thanks for the heads up! I just looked at that code yesterday while >>> analyzing bug 78608, wondering if it was safe. Now I know it isn't. >>> I think it might be best to simply hand code the expression instead >>> of taking a chance on abs. Let me take care of it today along with >>> 78608. >> >> I posted a bigger patch to fix this and other related problems on >> Friday (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html). >> In hindsight, I should have probably committed the fix for this >> on its own. Please let me know if this is blocking you and I'll >> commit this fix by itself today so you don't have to wait for >> the bigger patch to get reviewed and approved. > What's the concern with using std::abs? My concern, when I wrote the reply n Friday, was that not all C++98 implementations may get std::abs right, declare it in the right header, avoid defining the abs macro, or put it in namespace std. (IIRC, the standard itself wasn't quite right.) I also need to avoid calling abs with a TYPE_MIN argument because that's undefined and flagged by ubsan (as per the bug in the subject, though it was not a result of calling abs but rather that of negating it). Besides avoiding the undefined behavior in the compiler I also need diagnose it (in the program). The test case for it goes like this: int n = sprintf (0, 0, "%*i", INT_MIN, 0); where the INT_MIN is interpreted as the left justification flag followed by a positive width of -(unsigned long)INT_MIN. The problem is that the function (declared to return int0 is being asked to return INT_MAX + 1 which is undefined (in the program). Martin
On 12/05/2016 11:52 AM, Martin Sebor wrote: >> What's the concern with using std::abs? > > My concern, when I wrote the reply n Friday, was that not all C++98 > implementations may get std::abs right, declare it in the right header, > avoid defining the abs macro, or put it in namespace std. (IIRC, > the standard itself wasn't quite right.) > > I also need to avoid calling abs with a TYPE_MIN argument because > that's undefined and flagged by ubsan (as per the bug in the subject, > though it was not a result of calling abs but rather that of negating > it). I'm less concerned about the older C++ implementations as I am about the TYPE_MIN overflow. Thanks for clarifying. > > Besides avoiding the undefined behavior in the compiler I also need > diagnose it (in the program). The test case for it goes like this: > > int n = sprintf (0, 0, "%*i", INT_MIN, 0); > > where the INT_MIN is interpreted as the left justification flag > followed by a positive width of -(unsigned long)INT_MIN. The > problem is that the function (declared to return int0 is being > asked to return INT_MAX + 1 which is undefined (in the program). Understood. Thanks again. jeff
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -774,7 +774,7 @@ get_width_and_precision (const conversio if (spec.star_width) { if (TREE_CODE (spec.star_width) == INTEGER_CST) - width = abs (tree_to_shwi (spec.star_width)); + width = std::abs (tree_to_shwi (spec.star_width)); else width = HOST_WIDE_INT_MIN; }