Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Tue, 2015-04-21 at 13:26 -0700, Roland McGrath wrote:
> [PATCH] libdwfl: Make dwfl_error.c C99-kosher.
> 
> Signed-off-by: Roland McGrath <roland@hack.frob.com>
> ---
>  libdwfl/ChangeLog    |  7 +++++++
>  libdwfl/dwfl_error.c | 21 +++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
>
> +2015-04-21  Roland McGrath  <roland@hack.frob.com>
> +
> +	* dwfl_error.c (struct msgtable): Break type definition out of
> +	the 'msgtable' initializer.
> +	(msgtable): Make it a union of struct msgtable and a char array.
> +	(msgstr): Use the full-table char array rather than the msg_0 entry.

This is the last patch needed to get make check happy when using
gcc -fsanitize=undefined. Please do push it (or one of the variants
discussed in the thread).

Thanks,

Mark

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 179 bytes --]

> This is the last patch needed to get make check happy when using
> gcc -fsanitize=undefined. Please do push it (or one of the variants
> discussed in the thread).

Pushed.

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

On Thu, 2015-04-23 at 10:05 -0700, Roland McGrath wrote:
> > Something like can be done without unions:
> > 
> > #define msgstr (*(char (*)[sizeof msgtable])&msgtable)
> > 
> > AFACT it's well-defined C and permits to perform bound checking.
> 
> That's less clean than a union.

I like the union solution more too. Please do push it.

Thanks,

Mark

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On Thu, 2015-04-23 at 10:04 -0700, Roland McGrath wrote:
> > I know you are nostalgic :) But no need to assign copyright to Red Hat.
> > Just add your own copyright notice to the file.
> 
> I do not want to assert any personal copyright on anything I contribute.

But neither does Red Hat want to assert copyright over your
contribution :) The Signed-off-by line to track who did what is enough
to confirm others have the right to pass on the patch. But if your
employer wants you can also add a copyright line for them.

Cheers,

Mark

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

> Extra plus in front of 0? Why not just use sizeof(struct msgtable)?

Yeah, I guess that would be fine too.

> Something like can be done without unions:
> 
> #define msgstr (*(char (*)[sizeof msgtable])&msgtable)
> 
> AFACT it's well-defined C and permits to perform bound checking.

That's less clean than a union.

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

> I know you are nostalgic :) But no need to assign copyright to Red Hat.
> Just add your own copyright notice to the file.

I do not want to assert any personal copyright on anything I contribute.

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Alexander Cherepanov]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

On 2015-04-21 23:26, Roland McGrath wrote:
> I think it's cleaner to do:
>
> [PATCH] libdwfl: Make dwfl_error.c C99-kosher.
>
> Signed-off-by: Roland McGrath <roland-/Z5OmTQCD9xF6kxbq+BtvQ(a)public.gmane.org>
> ---
>   libdwfl/ChangeLog    |  7 +++++++
>   libdwfl/dwfl_error.c | 21 +++++++++++++++++----
>   2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> index d4cd3f5..045adb0 100644
> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,10 @@
> +2015-04-21  Roland McGrath  <roland-/Z5OmTQCD9xF6kxbq+BtvQ(a)public.gmane.org>
> +
> +	* dwfl_error.c (struct msgtable): Break type definition out of
> +	the 'msgtable' initializer.
> +	(msgtable): Make it a union of struct msgtable and a char array.
> +	(msgstr): Use the full-table char array rather than the msg_0 entry.
> +
>   2015-04-02  Mark Wielaard  <mjw-H+wXaHxf7aLQT0dZR+AlfA(a)public.gmane.org>
>
>   	* segment.c (insert): Check correct number of lookup_elts.
> diff --git a/libdwfl/dwfl_error.c b/libdwfl/dwfl_error.c
> index d9ca9e7..f46b160 100644
> --- a/libdwfl/dwfl_error.c
> +++ b/libdwfl/dwfl_error.c
> @@ -1,5 +1,5 @@
>   /* Error handling in libdwfl.
> -   Copyright (C) 2005-2010 Red Hat, Inc.
> +   Copyright (C) 2005-2015 Red Hat, Inc.
>      This file is part of elfutils.
>
>      This file is free software; you can redistribute it and/or modify
> @@ -54,18 +54,31 @@ dwfl_errno (void)
>   INTDEF (dwfl_errno)
>
>
> -static const struct msgtable
> +struct msgtable
>   {
>   #define DWFL_ERROR(name, text) char msg_##name[sizeof text];
>     DWFL_ERRORS
>   #undef	DWFL_ERROR
> +};
> +
> +static const union
> +{
> +  struct msgtable table;
> +  char strings[
> +#define DWFL_ERROR(name, text)	sizeof text +
> +	       DWFL_ERRORS
> +#undef	DWFL_ERROR
> +	       + 0];

Extra plus in front of 0? Why not just use sizeof(struct msgtable)?

>   } msgtable =
>     {
> +    .table =
> +    {
>   #define DWFL_ERROR(name, text) text,
> -  DWFL_ERRORS
> +      DWFL_ERRORS
>   #undef	DWFL_ERROR
> +    }
>     };
> -#define msgstr (&msgtable.msg_NOERROR[0])
> +#define msgstr (msgtable.strings)

Something like can be done without unions:

#define msgstr (*(char (*)[sizeof msgtable])&msgtable)

AFACT it's well-defined C and permits to perform bound checking.

-- 
Alexander Cherepanov

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

Hi Roland,

On Tue, 2015-04-21 at 13:26 -0700, Roland McGrath wrote:
> I think it's cleaner to do:
> 
> [PATCH] libdwfl: Make dwfl_error.c C99-kosher.
>
> +2015-04-21  Roland McGrath  <roland@hack.frob.com>
> +
> +	* dwfl_error.c (struct msgtable): Break type definition out of
> +	the 'msgtable' initializer.
> +	(msgtable): Make it a union of struct msgtable and a char array.
> +	(msgstr): Use the full-table char array rather than the msg_0 entry.

Yes, much nicer. I hadn't thought of using a union and expand the
DWFL_ERRORS twice. Nice trick.

> --- a/libdwfl/dwfl_error.c
> +++ b/libdwfl/dwfl_error.c
> @@ -1,5 +1,5 @@
>  /* Error handling in libdwfl.
> -   Copyright (C) 2005-2010 Red Hat, Inc.
> +   Copyright (C) 2005-2015 Red Hat, Inc.
>     This file is part of elfutils.

I know you are nostalgic :) But no need to assign copyright to Red Hat.
Just add your own copyright notice to the file.

Thanks,

Mark

Re: [PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

I think it's cleaner to do:

[PATCH] libdwfl: Make dwfl_error.c C99-kosher.

Signed-off-by: Roland McGrath <roland@hack.frob.com>
---
 libdwfl/ChangeLog    |  7 +++++++
 libdwfl/dwfl_error.c | 21 +++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index d4cd3f5..045adb0 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,10 @@
+2015-04-21  Roland McGrath  <roland@hack.frob.com>
+
+	* dwfl_error.c (struct msgtable): Break type definition out of
+	the 'msgtable' initializer.
+	(msgtable): Make it a union of struct msgtable and a char array.
+	(msgstr): Use the full-table char array rather than the msg_0 entry.
+
 2015-04-02  Mark Wielaard  <mjw@redhat.com>
 
 	* segment.c (insert): Check correct number of lookup_elts.
diff --git a/libdwfl/dwfl_error.c b/libdwfl/dwfl_error.c
index d9ca9e7..f46b160 100644
--- a/libdwfl/dwfl_error.c
+++ b/libdwfl/dwfl_error.c
@@ -1,5 +1,5 @@
 /* Error handling in libdwfl.
-   Copyright (C) 2005-2010 Red Hat, Inc.
+   Copyright (C) 2005-2015 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -54,18 +54,31 @@ dwfl_errno (void)
 INTDEF (dwfl_errno)
 
 
-static const struct msgtable
+struct msgtable
 {
 #define DWFL_ERROR(name, text) char msg_##name[sizeof text];
   DWFL_ERRORS
 #undef	DWFL_ERROR
+};
+
+static const union
+{
+  struct msgtable table;
+  char strings[
+#define DWFL_ERROR(name, text)	sizeof text +
+	       DWFL_ERRORS
+#undef	DWFL_ERROR
+	       + 0];
 } msgtable =
   {
+    .table =
+    {
 #define DWFL_ERROR(name, text) text,
-  DWFL_ERRORS
+      DWFL_ERRORS
 #undef	DWFL_ERROR
+    }
   };
-#define msgstr (&msgtable.msg_NOERROR[0])
+#define msgstr (msgtable.strings)
 
 static const uint_fast16_t msgidx[] =
 {

[PATCH] libdwfl: Fix wrong type to make gcc -fsanitize=undefined happy.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

gcc -fsanitize=undefined would warn about calling dwfl_error accessing
the error string array: elfutils/libdwfl/dwfl_error.c:156:60:
	runtime error: index 385 out of bounds for type 'char [9]'

This is because it thought we went beyond error string zero "no error",
which is indeed 9 chars. "Correct" the type of the pointer to make
ubsan happy.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdwfl/ChangeLog    | 4 ++++
 libdwfl/dwfl_error.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index d4cd3f5..90c0a04 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,7 @@
+2015-04-21  Mark Wielaard  <mjw@redhat.com>
+
+	* dwfl_error.c (msgstr): Define as const char *.
+
 2015-04-02  Mark Wielaard  <mjw@redhat.com>
 
 	* segment.c (insert): Check correct number of lookup_elts.
diff --git a/libdwfl/dwfl_error.c b/libdwfl/dwfl_error.c
index d9ca9e7..c3648c0 100644
--- a/libdwfl/dwfl_error.c
+++ b/libdwfl/dwfl_error.c
@@ -65,7 +65,7 @@ static const struct msgtable
   DWFL_ERRORS
 #undef	DWFL_ERROR
   };
-#define msgstr (&msgtable.msg_NOERROR[0])
+#define msgstr ((const char *) &msgtable)
 
 static const uint_fast16_t msgidx[] =
 {
-- 
2.1.0