Re: [PATCH v3] Use fallthrough attribute

[Mark Wielaard]

Hi Joshua,

On Fri, Feb 09, 2018 at 10:27:18AM -0600, Joshua Watt wrote:
> Use __attribute__ ((fallthrough)) to indicate switch case fall through
> instead of a comment. This ensure that the fallthrough warning is not
> triggered even if the file is pre-processed (hence stripping the
> comments) before it is compiled.
> 
> The actual fallback implementation is hidden behind a FALLBACK macro in
> case the compiler doesn't support it.
> 
> Finally, the -Wimplict-fallthrough warning was upgraded to only allow
> the attribute to satisfy it; a comment alone is no longer sufficient.

Thanks, this look good.
I added ChangeLog entries and pushed it to master.

Cheers,

Mark

[PATCH] libdw: Handle DWARF5 DW_FORM_implicit_const. Add dwarf_getabbrevattr_data.

[Mark Wielaard]

Handle the new DW_FORM_implicit_const. The value of this form is embedded
in the abbrev data (as sleb128) and not in the info DIE data. This also
adds a new function dwarf_getabbrevattr_data which allows getting any
data/value associated with a form. eu-readelf will use this new function
to show the DW_FORM_implicit_const value.

Signed-off-by: Mark Wielaard 
---
 libdw/ChangeLog | 19 +++
 libdw/dwarf_child.c | 14 +-
 libdw/dwarf_formsdata.c |  7 ++-
 libdw/dwarf_formudata.c |  7 ++-
 libdw/dwarf_getabbrev.c |  7 +++
 libdw/dwarf_getabbrevattr.c | 22 --
 libdw/dwarf_getattrs.c  | 11 ++-
 libdw/dwarf_hasattr.c   |  6 ++
 libdw/libdw.h   |  6 ++
 libdw/libdw.map |  1 +
 libdw/libdwP.h  |  1 +
 libdw/memory-access.h   | 19 +++
 src/ChangeLog   |  6 ++
 src/readelf.c   | 15 +--
 14 files changed, 129 insertions(+), 12 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 65e4ddc..f52ce58 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,22 @@
+2018-02-09  Mark Wielaard  
+
+   * dwarf_child.c (__libdw_find_attr): Handle DW_FORM_implicit_const.
+   * dwarf_formsdata.c (dwarf_formsdata): Likewise.
+   * dwarf_formudata.c (dwarf_formudata): Likewise.
+   * dwarf_getabbrev.c (__libdw_getabbrev): Likewise.
+   * dwarf_getattrs.c (dwarf_getattrs): Likewise.
+   * dwarf_hasattr.c (dwarf_hasattr): Likewise.
+   * dwarf_getabbrevattr.c (dwarf_getabbrevattr_data): New function
+   that will also return any data associated with the abbrev. Which
+   currently is only for DW_FORM_implicit_const. Based on...
+   (dwarf_getabbrevattr): ... this function. Which now just calls
+   dwarf_getabbrevattr_data.
+   * libdw.h (dwarf_getabbrevattr_data): Declare new function.
+   * libdw.map (ELFUTILS_0.170): Add dwarf_getabbrevattr_data.
+   * libdwP.h (dwarf_getabbrevattr_data): INTDECL.
+   * memory-access.h (__libdw_get_sleb128_unchecked): New inlined
+   function based on __libdw_get_uleb128_unchecked.
+
 2018-02-08  Mark Wielaard  
 
* dwarf.h: Add DWARF5 DW_FORMs.
diff --git a/libdw/dwarf_child.c b/libdw/dwarf_child.c
index 248338e..9446b88 100644
--- a/libdw/dwarf_child.c
+++ b/libdw/dwarf_child.c
@@ -77,7 +77,12 @@ __libdw_find_attr (Dwarf_Die *die, unsigned int search_name,
  if (formp != NULL)
*formp = attr_form;
 
- return (unsigned char *) readp;
+ /* Normally the attribute data comes from the DIE/info,
+except for implicit_form, where it comes from the abbrev.  */
+ if (attr_form == DW_FORM_implicit_const)
+   return (unsigned char *) attrp;
+ else
+   return (unsigned char *) readp;
}
 
   /* Skip over the rest of this attribute (if there is any).  */
@@ -92,6 +97,13 @@ __libdw_find_attr (Dwarf_Die *die, unsigned int search_name,
 
  // __libdw_form_val_len will have done a bounds check.
  readp += len;
+
+ // If the value is in the abbrev data, skip it.
+ if (attr_form == DW_FORM_implicit_const)
+   {
+ int64_t attr_value __attribute__((__unused__));
+ get_sleb128_unchecked (attr_value, attrp);
+   }
}
 }
 
diff --git a/libdw/dwarf_formsdata.c b/libdw/dwarf_formsdata.c
index bc2b508..def32c9 100644
--- a/libdw/dwarf_formsdata.c
+++ b/libdw/dwarf_formsdata.c
@@ -1,5 +1,5 @@
 /* Return signed constant represented by attribute.
-   Copyright (C) 2003, 2005, 2014 Red Hat, Inc.
+   Copyright (C) 2003, 2005, 2014, 2017 Red Hat, Inc.
This file is part of elfutils.
Written by Ulrich Drepper , 2003.
 
@@ -86,6 +86,11 @@ dwarf_formsdata (Dwarf_Attribute *attr, Dwarf_Sword 
*return_sval)
   get_uleb128 (*return_sval, datap, endp);
   break;
 
+case DW_FORM_implicit_const:
+  // The data comes from the abbrev, which has been bounds checked.
+  get_sleb128_unchecked (*return_sval, datap);
+  break;
+
 default:
   __libdw_seterrno (DWARF_E_NO_CONSTANT);
   return -1;
diff --git a/libdw/dwarf_formudata.c b/libdw/dwarf_formudata.c
index e41981a..9c1644e 100644
--- a/libdw/dwarf_formudata.c
+++ b/libdw/dwarf_formudata.c
@@ -1,5 +1,5 @@
 /* Return unsigned constant represented by attribute.
-   Copyright (C) 2003-2012, 2014 Red Hat, Inc.
+   Copyright (C) 2003-2012, 2014, 2017 Red Hat, Inc.
This file is part of elfutils.
Written by Ulrich Drepper , 2003.
 
@@ -221,6 +221,11 @@ dwarf_formudata (Dwarf_Attribute *attr, Dwarf_Word 
*return_uval)
   get_uleb128 (*return_uval, datap, endp);
   break;
 
+case DW_FORM_implicit_const:
+  // The data comes from the abbrev, which has been bounds checked.
+  

Re: [PATCH v2] Add fallthrough attributes

[Mark Wielaard]

On Fri, Feb 09, 2018 at 10:08:09AM +0100, Ulf Hermann wrote:
> > [...]
> > +#ifdef HAVE_FALLTHROUGH
> > +  __attribute__ ((fallthrough));
> > +#endif
> > [...]
> 
> I would like to see this stanza wrapped in a macro, so that we only have one 
> "#ifdef HAVE_FALLTHROUGH" in the code, not another one in every place we want 
> to fall through. See the "internal_function" macro defined in lib/eu-config.h 
> for a similar case.

Agreed. Having 4 lines for a fallthrough instead of 1 is really too
much. Also could you explain a bit more why you would like this?
The advantage of the comments really is that they should work
everywhere.

If the comment really doesn't work in your situation maybe we could do
like gnulib did:
http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=11fdf80b21f2b40a10687b9a3d16c852b19d512c

The idea is that those versions of GCC that support
-Wimplicit-fallthrough also have support for the __attribute__
((fallthrough)) statement. So they can always be used together.

Cheers,

Mark

Re: [PATCH v2] Add fallthrough attributes

[Ulf Hermann]

> [...]
> +#ifdef HAVE_FALLTHROUGH
> +  __attribute__ ((fallthrough));
> +#endif
> [...]

I would like to see this stanza wrapped in a macro, so that we only have one 
"#ifdef HAVE_FALLTHROUGH" in the code, not another one in every place we want 
to fall through. See the "internal_function" macro defined in lib/eu-config.h 
for a similar case.

Ulf