rfc: patch to remove undefined behavior

--------------BD3B21F4B8AA02816830E084
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

This silences a bunch of compiler warnings that are overwhelming other 
warnings.  I'm not sure if there is a preferable way to do this, so I'm 
posting it for comment

--------------BD3B21F4B8AA02816830E084
Content-Type: text/x-patch;
 name="0001-regen-warnings.pl-Fix-undefined-C-behavior.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-regen-warnings.pl-Fix-undefined-C-behavior.patch"

From f23e68276d03897e44b10234c4b4fa9edb23893a Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Sun, 30 Dec 2018 21:29:07 -0700
Subject: [PATCH] regen/warnings.pl: Fix undefined C behavior

This fixes compiler warnings "performing pointer arithmetic on a null
pointer has undefined behavior"

There are several ways to fix this.  This one was suggested by
Tomasz Konojacki++.  Instead of trying to point to address 1 and 2, two
variables are created, and we point to them.  const is cast away.
---
 perl.h            | 5 +++++
 regen/warnings.pl | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/perl.h b/perl.h
index 6d68c975fa..ccb4faa87a 100644
--- a/perl.h
+++ b/perl.h
@@ -4454,6 +4454,11 @@ EXTCONST char PL_Zero[]
 EXTCONST char PL_hexdigit[]
   INIT("0123456789abcdef0123456789ABCDEF");
 
+EXTCONST STRLEN PL_WARN_ALL
+  INIT(0);
+EXTCONST STRLEN PL_WARN_NONE
+  INIT(0);
+
 /* This is constant on most architectures, a global on OS/2 */
 #ifndef OS2
 EXTCONST char PL_sh_path[]
diff --git a/regen/warnings.pl b/regen/warnings.pl
index 9c01e6762f..d244160b3e 100644
--- a/regen/warnings.pl
+++ b/regen/warnings.pl
@@ -322,8 +322,8 @@ my ($index, $warn_size);
 #define G_WARN_ALL_MASK		(G_WARN_ALL_ON|G_WARN_ALL_OFF)
 
 #define pWARN_STD		NULL
-#define pWARN_ALL		(((STRLEN*)0)+1)    /* use warnings 'all' */
-#define pWARN_NONE		(((STRLEN*)0)+2)    /* no  warnings 'all' */
+#define pWARN_ALL		(STRLEN *) &PL_WARN_ALL    /* use warnings 'all' */
+#define pWARN_NONE		(STRLEN *) &PL_WARN_NONE   /* no  warnings 'all' */
 
 #define specialWARN(x)		((x) == pWARN_STD || (x) == pWARN_ALL ||	\
 				 (x) == pWARN_NONE)
-- 
2.17.1


--------------BD3B21F4B8AA02816830E084--
0
public
12/31/2018 4:58:23 AM
perl.perl5.porters 47546 articles. 1 followers. Follow

6 Replies
34 Views

Similar Articles

[PageSpeed] 38

On 12/30/18 11:58 PM, Karl Williamson wrote:
> This silences a bunch of compiler warnings that are overwhelming other 
> warnings.  I'm not sure if there is a preferable way to do this, so I'm 
> posting it for comment

I would *really* like to see this patch (or a patch with equivalent 
effect) applied because it would silence the majority of build-time 
warnings which we are getting in smoke-tests and which make it difficult 
to spot the emergence of new, more significant warnings.

I raised this in https://rt.perl.org/Ticket/Display.html?id=133223. 
See, for example, the tally of warnings in this comment:

https://rt.perl.org/Ticket/Display.html?id=133223#txn-1556136

However, when I applied the patch I got 2 test failures in make 
test_porting: porting/globvar.t and porting/warnings.t.

The second of those was easily corrected by running ./perl 
regen/warnings.pl.

The first of those, to be properly corrected, needs to have a 
determination as to whether PL_WARN_ADD and PL_WARN_NONE are to be 
considered "exported" or "nonexported".  I don't know how to do that so, 
for the purpose of getting out a smoke-me branch I classified them as 
nonexported in t/porting/globvar.t.  So the following branch is 
available for smoke-testing to gauge the impact of the tomas/khw patch 
on the number of build-time warnings generated:

smoke-me/jkeenan/khw/undefined-behavior-warnings

Karl, you might want to consider making this into an RT, as we could 
then link to the META ticket on build-time warnings.

Thank you very much.
Jim Keenan
0
jkeenan
12/31/2018 3:02:29 PM
--------------2A9335738E36AA7AEE83DF18
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable

On 12/31/18 8:02 AM, James E Keenan wrote:
> On 12/30/18 11:58 PM, Karl Williamson wrote:
>> This silences a bunch of compiler warnings that are overwhelming other=
=20
>> warnings.=C2=A0 I'm not sure if there is a preferable way to do this, =
so=20
>> I'm posting it for comment
>=20
> I would *really* like to see this patch (or a patch with equivalent=20
> effect) applied because it would silence the majority of build-time=20
> warnings which we are getting in smoke-tests and which make it difficul=
t=20
> to spot the emergence of new, more significant warnings.
>=20
> I raised this in https://rt.perl.org/Ticket/Display.html?id=3D133223. S=
ee,=20
> for example, the tally of warnings in this comment:
>=20
> https://rt.perl.org/Ticket/Display.html?id=3D133223#txn-1556136
>=20
> However, when I applied the patch I got 2 test failures in make=20
> test_porting: porting/globvar.t and porting/warnings.t.
>=20
> The second of those was easily corrected by running ./perl=20
> regen/warnings.pl.
>=20
> The first of those, to be properly corrected, needs to have a=20
> determination as to whether PL_WARN_ADD and PL_WARN_NONE are to be=20
> considered "exported" or "nonexported".=C2=A0 I don't know how to do th=
at so,=20
> for the purpose of getting out a smoke-me branch I classified them as=20
> nonexported in t/porting/globvar.t.=C2=A0 So the following branch is=20
> available for smoke-testing to gauge the impact of the tomas/khw patch=20
> on the number of build-time warnings generated:
>=20
> smoke-me/jkeenan/khw/undefined-behavior-warnings
>=20
> Karl, you might want to consider making this into an RT, as we could=20
> then link to the META ticket on build-time warnings.
>=20
> Thank you very much.
> Jim Keenan
>=20

I forgot to include all files in the patch I posted earlier.  This is a=20
corrected version, attached.

--------------2A9335738E36AA7AEE83DF18
Content-Type: text/x-patch;
 name="0001-regen-warnings.pl-Fix-undefined-C-behavior.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-regen-warnings.pl-Fix-undefined-C-behavior.patch"

From 5cceff4a57c18a971ab46b75df561881eb3ce363 Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Sun, 30 Dec 2018 21:29:07 -0700
Subject: [PATCH] regen/warnings.pl: Fix undefined C behavior

This fixes compiler warnings "performing pointer arithmetic on a null
pointer has undefined behavior"

There are several ways to fix this.  This one was suggested by
Tomasz Konojacki++.  Instead of trying to point to address 1 and 2, two
variables are created, and we point to them.  const is cast away.
---
 globvar.sym       | 2 ++
 perl.h            | 5 +++++
 regen/warnings.pl | 4 ++--
 warnings.h        | 4 ++--
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/globvar.sym b/globvar.sym
index 6fb387ca0a..476f4ca095 100644
--- a/globvar.sym
+++ b/globvar.sym
@@ -83,4 +83,6 @@ PL_warn_nl
 PL_warn_nosemi
 PL_warn_reserved
 PL_warn_uninit
+PL_WARN_ALL
+PL_WARN_NONE
 PL_watch_pvx
diff --git a/perl.h b/perl.h
index 6d68c975fa..ccb4faa87a 100644
--- a/perl.h
+++ b/perl.h
@@ -4454,6 +4454,11 @@ EXTCONST char PL_Zero[]
 EXTCONST char PL_hexdigit[]
   INIT("0123456789abcdef0123456789ABCDEF");
 
+EXTCONST STRLEN PL_WARN_ALL
+  INIT(0);
+EXTCONST STRLEN PL_WARN_NONE
+  INIT(0);
+
 /* This is constant on most architectures, a global on OS/2 */
 #ifndef OS2
 EXTCONST char PL_sh_path[]
diff --git a/regen/warnings.pl b/regen/warnings.pl
index 9c01e6762f..d244160b3e 100644
--- a/regen/warnings.pl
+++ b/regen/warnings.pl
@@ -322,8 +322,8 @@ my ($index, $warn_size);
 #define G_WARN_ALL_MASK		(G_WARN_ALL_ON|G_WARN_ALL_OFF)
 
 #define pWARN_STD		NULL
-#define pWARN_ALL		(((STRLEN*)0)+1)    /* use warnings 'all' */
-#define pWARN_NONE		(((STRLEN*)0)+2)    /* no  warnings 'all' */
+#define pWARN_ALL		(STRLEN *) &PL_WARN_ALL    /* use warnings 'all' */
+#define pWARN_NONE		(STRLEN *) &PL_WARN_NONE   /* no  warnings 'all' */
 
 #define specialWARN(x)		((x) == pWARN_STD || (x) == pWARN_ALL ||	\
 				 (x) == pWARN_NONE)
diff --git a/warnings.h b/warnings.h
index e0c12ed403..58f52272de 100644
--- a/warnings.h
+++ b/warnings.h
@@ -18,8 +18,8 @@
 #define G_WARN_ALL_MASK		(G_WARN_ALL_ON|G_WARN_ALL_OFF)
 
 #define pWARN_STD		NULL
-#define pWARN_ALL		(((STRLEN*)0)+1)    /* use warnings 'all' */
-#define pWARN_NONE		(((STRLEN*)0)+2)    /* no  warnings 'all' */
+#define pWARN_ALL		(STRLEN *) &PL_WARN_ALL    /* use warnings 'all' */
+#define pWARN_NONE		(STRLEN *) &PL_WARN_NONE   /* no  warnings 'all' */
 
 #define specialWARN(x)		((x) == pWARN_STD || (x) == pWARN_ALL ||	\
 				 (x) == pWARN_NONE)
-- 
2.17.1


--------------2A9335738E36AA7AEE83DF18--
0
public
12/31/2018 3:58:04 PM
On 12/31/18 10:58 AM, Karl Williamson wrote:
> On 12/31/18 8:02 AM, James E Keenan wrote:
>> On 12/30/18 11:58 PM, Karl Williamson wrote:
>>> This silences a bunch of compiler warnings that are overwhelming 
>>> other warnings.  I'm not sure if there is a preferable way to do 
>>> this, so I'm posting it for comment
>>
>> I would *really* like to see this patch (or a patch with equivalent 
>> effect) applied because it would silence the majority of build-time 
>> warnings which we are getting in smoke-tests and which make it 
>> difficult to spot the emergence of new, more significant warnings.
>>
>> I raised this in https://rt.perl.org/Ticket/Display.html?id=133223. 
>> See, for example, the tally of warnings in this comment:
>>
>> https://rt.perl.org/Ticket/Display.html?id=133223#txn-1556136
>>
>> However, when I applied the patch I got 2 test failures in make 
>> test_porting: porting/globvar.t and porting/warnings.t.
>>
>> The second of those was easily corrected by running ./perl 
>> regen/warnings.pl.
>>
>> The first of those, to be properly corrected, needs to have a 
>> determination as to whether PL_WARN_ADD and PL_WARN_NONE are to be 
>> considered "exported" or "nonexported".  I don't know how to do that 
>> so, for the purpose of getting out a smoke-me branch I classified them 
>> as nonexported in t/porting/globvar.t.  So the following branch is 
>> available for smoke-testing to gauge the impact of the tomas/khw patch 
>> on the number of build-time warnings generated:
>>
>> smoke-me/jkeenan/khw/undefined-behavior-warnings
>>
>> Karl, you might want to consider making this into an RT, as we could 
>> then link to the META ticket on build-time warnings.
>>
>> Thank you very much.
>> Jim Keenan
>>
> 
> I forgot to include all files in the patch I posted earlier.  This is a 
> corrected version, attached.

Available for smoking in this branch:

smoke-me/jkeenan/khw/undefined-behavior-warnings-2nd
0
jkeenan
12/31/2018 7:58:45 PM
On 12/31/18 10:58 AM, Karl Williamson wrote:
> On 12/31/18 8:02 AM, James E Keenan wrote:
>> On 12/30/18 11:58 PM, Karl Williamson wrote:
>>> This silences a bunch of compiler warnings that are overwhelming 
>>> other warnings.  I'm not sure if there is a preferable way to do 
>>> this, so I'm posting it for comment
>>
>> I would *really* like to see this patch (or a patch with equivalent 
>> effect) applied because it would silence the majority of build-time 
>> warnings which we are getting in smoke-tests and which make it 
>> difficult to spot the emergence of new, more significant warnings.
>>
>> I raised this in https://rt.perl.org/Ticket/Display.html?id=133223. 
>> See, for example, the tally of warnings in this comment:
>>
>> https://rt.perl.org/Ticket/Display.html?id=133223#txn-1556136
>>
>> However, when I applied the patch I got 2 test failures in make 
>> test_porting: porting/globvar.t and porting/warnings.t.
>>
>> The second of those was easily corrected by running ./perl 
>> regen/warnings.pl.
>>
>> The first of those, to be properly corrected, needs to have a 
>> determination as to whether PL_WARN_ADD and PL_WARN_NONE are to be 
>> considered "exported" or "nonexported".  I don't know how to do that 
>> so, for the purpose of getting out a smoke-me branch I classified them 
>> as nonexported in t/porting/globvar.t.  So the following branch is 
>> available for smoke-testing to gauge the impact of the tomas/khw patch 
>> on the number of build-time warnings generated:
>>
>> smoke-me/jkeenan/khw/undefined-behavior-warnings
>>
>> Karl, you might want to consider making this into an RT, as we could 
>> then link to the META ticket on build-time warnings.
>>
>> Thank you very much.
>> Jim Keenan
>>
> 
> I forgot to include all files in the patch I posted earlier.  This is a 
> corrected version, attached.

Reviewing the results at 
http://perl.develop-help.com/?b=smoke-me%2Fjkeenan%2Fkhw%2Fundefined-behavior-warnings-2nd, 
I'd say these patches are good to be applied to blead.

The only smoke-test runs that are failing in that branch are those that 
are always or almost always failing in blead as well.  For example, if 
you go to http://perl5.test-smoke.org/search and use drop-downs to 
select netbsd 8.0 with g++-5.5.0 as compiler, you see that blead and 
current smoke-me branches are failing.  So the patches do no harm.

And the patches do a tremendous amount of good with respect to cleaning 
up build-time warnings.  Consider, for example, these two smoke-test 
reports from Carlos, both on FreeBSD-13, both with clang-6.0.1 as the C 
compiler.  In each case, we search for the warnings category 
"-Wnull-pointer-arithmetic".

http://perl5.test-smoke.org/report/77389
branch: blead
count of "-Wnull-pointer-arithmetic" warnings: 94

http://perl5.test-smoke.org/report/77376
branch: smoke-me/jkeenan/khw/undefined-behavior-warnings-2nd
count of "-Wnull-pointer-arithmetic" warnings:  0

So, +1 from me to apply branches (or merge that branch).

Thank you very much.
Jim Keenan
0
jkeenan
1/3/2019 3:01:57 PM
On Mon, 31 Dec 2018, James E Keenan wrote:

>
> Available for smoking in this branch:
>
> smoke-me/jkeenan/khw/undefined-behavior-warnings-2nd

Speaking of undefined behavior, I recently realized UBSan only prints a 
warning by default so I've switched my "sanitize=undefined" smokes to 
abort on detection, like AddressSanitizer does.  I updated the user_note 
as well to indicate the change, although not until some had already 
started. (And briefly broke the Darwin ASan build due to similar change.)

That's more of an "FYI" smoke profile, so I don't expect it to be clean.


-- 
George Greer
0
perl
1/4/2019 5:57:16 AM
On 1/3/19 8:01 AM, James E Keenan wrote:
> On 12/31/18 10:58 AM, Karl Williamson wrote:
>> On 12/31/18 8:02 AM, James E Keenan wrote:
>>> On 12/30/18 11:58 PM, Karl Williamson wrote:
>>>> This silences a bunch of compiler warnings that are overwhelming=20
>>>> other warnings.=C2=A0 I'm not sure if there is a preferable way to d=
o=20
>>>> this, so I'm posting it for comment
>>>
>>> I would *really* like to see this patch (or a patch with equivalent=20
>>> effect) applied because it would silence the majority of build-time=20
>>> warnings which we are getting in smoke-tests and which make it=20
>>> difficult to spot the emergence of new, more significant warnings.
>>>
>>> I raised this in https://rt.perl.org/Ticket/Display.html?id=3D133223.=
=20
>>> See, for example, the tally of warnings in this comment:
>>>
>>> https://rt.perl.org/Ticket/Display.html?id=3D133223#txn-1556136
>>>
>>> However, when I applied the patch I got 2 test failures in make=20
>>> test_porting: porting/globvar.t and porting/warnings.t.
>>>
>>> The second of those was easily corrected by running ./perl=20
>>> regen/warnings.pl.
>>>
>>> The first of those, to be properly corrected, needs to have a=20
>>> determination as to whether PL_WARN_ADD and PL_WARN_NONE are to be=20
>>> considered "exported" or "nonexported".=C2=A0 I don't know how to do =
that=20
>>> so, for the purpose of getting out a smoke-me branch I classified=20
>>> them as nonexported in t/porting/globvar.t.=C2=A0 So the following br=
anch=20
>>> is available for smoke-testing to gauge the impact of the tomas/khw=20
>>> patch on the number of build-time warnings generated:
>>>
>>> smoke-me/jkeenan/khw/undefined-behavior-warnings
>>>
>>> Karl, you might want to consider making this into an RT, as we could=20
>>> then link to the META ticket on build-time warnings.
>>>
>>> Thank you very much.
>>> Jim Keenan
>>>
>>
>> I forgot to include all files in the patch I posted earlier.=C2=A0 Thi=
s is=20
>> a corrected version, attached.
>=20
> Reviewing the results at=20
> http://perl.develop-help.com/?b=3Dsmoke-me%2Fjkeenan%2Fkhw%2Fundefined-=
behavior-warnings-2nd,=20
> I'd say these patches are good to be applied to blead.
>=20
> The only smoke-test runs that are failing in that branch are those that=
=20
> are always or almost always failing in blead as well.=C2=A0 For example=
, if=20
> you go to http://perl5.test-smoke.org/search and use drop-downs to=20
> select netbsd 8.0 with g++-5.5.0 as compiler, you see that blead and=20
> current smoke-me branches are failing.=C2=A0 So the patches do no harm.
>=20
> And the patches do a tremendous amount of good with respect to cleaning=
=20
> up build-time warnings.=C2=A0 Consider, for example, these two smoke-te=
st=20
> reports from Carlos, both on FreeBSD-13, both with clang-6.0.1 as the C=
=20
> compiler.=C2=A0 In each case, we search for the warnings category=20
> "-Wnull-pointer-arithmetic".
>=20
> http://perl5.test-smoke.org/report/77389
> branch: blead
> count of "-Wnull-pointer-arithmetic" warnings: 94
>=20
> http://perl5.test-smoke.org/report/77376
> branch: smoke-me/jkeenan/khw/undefined-behavior-warnings-2nd
> count of "-Wnull-pointer-arithmetic" warnings:=C2=A0 0
>=20
> So, +1 from me to apply branches (or merge that branch).
>=20
> Thank you very much.
> Jim Keenan
>=20

Since there has been no dissent on applying this patch, I did so in=20
8c165a32b7cc4f2a147a37c920a96f1b09b2386d
0
public
1/5/2019 5:43:56 PM
Reply: