This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

nonnull Re: [PATCH] Fix type warnings from clang compiler.


Hi,

On Wed, 2015-09-09 at 14:12 -0700, Chih-hung Hsieh wrote:
> From 862bacf11cacc734f854f81b64edde23465228c7 Mon Sep 17 00:00:00 2001
> From: Chih-Hung Hsieh <chh@google.com>
> Date: Wed, 9 Sep 2015 12:32:07 -0700
> Subject: [PATCH] Remove redundant NULL tests.
> 
> Clang gives warning on redundant NULL tests of parameters
> that are declared with __nonnull_attribute__.

Thanks. This seems a very useful warning.
I submitted a patch to GCC to warn about the same:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00626.html

I believe your fixes are correct except for one, which I believe is a
bug in our nonnull definition in the header file. And there is one I am
not 100% sure of. I kept your fix, but if anybody has a comment on it,
that would be good.

For libebl we always require a valid Ebl * be passed in.
Which is fine, since this really is just an internal library.
All the libebl changes are because of this, the ebl NULL check is
clearly wrong.

In general we require "result" pointers being passed in to be nonnull,
except if they are really optional results. That makes it easy to see
who is responsible for allocating and releasing the result resources
(the caller). Here the NULL checks are clearly bogus. We didn't intend
to support that. All, but two, of the non-libebl fixes are for this
case.

For the non-ebl library functions we normally allow passing in NULL as
input and then returning NULL as result to allow "chaining errors". That
way you can just call several functions in a row and if any one fails
that will set an error and the rest will "silently fail" because they
see the NULL being passed in and return NULL too. That way you don't
need to check for failure on every call, just on the last one. Making
error handling slightly nicer.

This is the case for dwfl_module_getelf () which has the following
definition in libdwfl.h:

extern Elf *dwfl_module_getelf (Dwfl_Module *, GElf_Addr *bias)
  __nonnull_attribute__ (1, 2);

I believe in this case the first argument should not be nonnull.
Since we would expect people to be able to chain for example the
dwfl_addrmodule () call followed by a dwfl_module_getelf () call.

So the correct fix in that case seems to be the removal of the nonnull
attribute for the first argument and leave the NULL check against mod in
the implementation.

There is one other case where I am not totally sure the nonnull
attribute was intended is for the internal function
__libdw_visit_scopes. It was rewritten from having just one (pre)visit
function to having both a previsit and postvisit callback function. In
all cases we are using it with a previsit one. But maybe the intention
was that you could only supply a postvisit one and keep the previsit one
NULL? I kept your fix for now, since it is an internal only function
anyway, but reindented the code after the if statement removal.

Slightly updated patch attached.

Thanks,

Mark
From d58040fb009fe0f6d5304e5402b2883df84594fa Mon Sep 17 00:00:00 2001
From: Chih-Hung Hsieh <chh@google.com>
Date: Wed, 9 Sep 2015 12:32:07 -0700
Subject: [PATCH] Remove redundant NULL tests.

Clang gives warning on redundant NULL tests of parameters
that are declared with __nonnull_attribute__.

Signed-off-by: Chih-Hung Hsieh <chh@google.com>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog                 |  7 +++++++
 libdw/dwarf_macro_getsrcfiles.c |  4 +---
 libdw/dwarf_siblingof.c         |  3 +--
 libdw/libdw_visit_scopes.c      | 14 ++++++--------
 libdwfl/ChangeLog               |  8 ++++++++
 libdwfl/dwfl_frame.c            |  3 ++-
 libdwfl/libdwfl.h               |  2 +-
 libebl/ChangeLog                |  8 ++++++++
 libebl/ebldwarftoregno.c        |  3 +--
 libebl/eblinitreg.c             |  3 ++-
 libebl/eblnormalizepc.c         |  3 ++-
 libebl/eblunwind.c              |  3 ++-
 12 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 96f8d90..13beefc 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,10 @@
+2015-09-09  Chih-Hung Hsieh  <chh@google.com>
+
+	* dwarf_macro_getsrcfiles.c (dwarf_macro_getsrcfiles): Remove
+	redundant NULL tests on parameters declared with __nonnull_attribute__.
+	* dwarf_siblingof.c (dwarf_siblingof): Likewise.
+	* libdw_visit_scopes.c (__libdw_visit_scopes): Likewise.
+
 2015-09-04  Chih-Hung Hsieh  <chh@google.com>
 	    Mark Wielaard  <mjw@redhat.com>
 
diff --git a/libdw/dwarf_macro_getsrcfiles.c b/libdw/dwarf_macro_getsrcfiles.c
index cc19043..3b1794b 100644
--- a/libdw/dwarf_macro_getsrcfiles.c
+++ b/libdw/dwarf_macro_getsrcfiles.c
@@ -36,9 +36,7 @@ int
 dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro,
 			 Dwarf_Files **files, size_t *nfiles)
 {
-  if (macro == NULL)
-    return -1;
-
+  /* macro is declared NN */
   Dwarf_Macro_Op_Table *const table = macro->table;
   if (table->files == NULL)
     {
diff --git a/libdw/dwarf_siblingof.c b/libdw/dwarf_siblingof.c
index e598ae4..0dafc17 100644
--- a/libdw/dwarf_siblingof.c
+++ b/libdw/dwarf_siblingof.c
@@ -45,8 +45,7 @@ dwarf_siblingof (die, result)
   if (die == NULL)
     return -1;
 
-  if (result == NULL)
-    return -1;
+  /* result is declared NN */
 
   if (result != die)
     result->addr = NULL;
diff --git a/libdw/libdw_visit_scopes.c b/libdw/libdw_visit_scopes.c
index ac7e853..d0b5134 100644
--- a/libdw/libdw_visit_scopes.c
+++ b/libdw/libdw_visit_scopes.c
@@ -138,24 +138,22 @@ __libdw_visit_scopes (depth, root, imports, previsit, postvisit, arg)
 
 	child.prune = false;
 
-	if (previsit != NULL)
-	  {
-	    int result = (*previsit) (depth + 1, &child, arg);
-	    if (result != DWARF_CB_OK)
-	      return result;
-	  }
+	/* previsit is declared NN */
+	int result = (*previsit) (depth + 1, &child, arg);
+	if (result != DWARF_CB_OK)
+	  return result;
 
 	if (!child.prune && may_have_scopes (&child.die)
 	    && INTUSE(dwarf_haschildren) (&child.die))
 	  {
-	    int result = recurse ();
+	    result = recurse ();
 	    if (result != DWARF_CB_OK)
 	      return result;
 	  }
 
 	if (postvisit != NULL)
 	  {
-	    int result = (*postvisit) (depth + 1, &child, arg);
+	    result = (*postvisit) (depth + 1, &child, arg);
 	    if (result != DWARF_CB_OK)
 	      return result;
 	  }
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 2c76369..c7f2b08 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,11 @@
+2015-09-09  Chih-Hung Hsieh  <chh@google.com>
+	    Mark Wielaard  <mjw@redhat.com>
+
+	* dwfl_frame.c (dwfl_attach_state): Remove redundant NULL tests
+	on parameters declared with __nonnull_attribute__.
+	* libdwfl.h (dwfl_module_getelf): Don't mark first argument as
+	nonnull.
+
 2015-09-04  Chih-Hung Hsieh  <chh@google.com
 
 	* frame_unwind.c (expr_eval): Use llabs instead of abs.
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index f6f86c0..a91a1d6 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -143,7 +143,8 @@ dwfl_attach_state (Dwfl *dwfl, Elf *elf, pid_t pid,
 
   /* Reset any previous error, we are just going to try again.  */
   dwfl->attacherr = DWFL_E_NOERROR;
-  if (thread_callbacks == NULL || thread_callbacks->next_thread == NULL
+  /* thread_callbacks is declared NN */
+  if (thread_callbacks->next_thread == NULL
       || thread_callbacks->set_initial_registers == NULL)
     {
       dwfl->attacherr = DWFL_E_INVALID_ARGUMENT;
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index 1098c83..aea8b99 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -423,7 +423,7 @@ extern int dwfl_validate_address (Dwfl *dwfl,
    with the difference between addresses within the loaded module
    and those in symbol tables or Dwarf information referring to it.  */
 extern Elf *dwfl_module_getelf (Dwfl_Module *, GElf_Addr *bias)
-  __nonnull_attribute__ (1, 2);
+  __nonnull_attribute__ (2);
 
 /* Return the number of symbols in the module's symbol table,
    or -1 for errors.  */
diff --git a/libebl/ChangeLog b/libebl/ChangeLog
index 60ae566..aab0857 100644
--- a/libebl/ChangeLog
+++ b/libebl/ChangeLog
@@ -1,3 +1,11 @@
+2015-09-09  Chih-Hung Hsieh  <chh@google.com>
+
+	* ebldwarftoregno.c (ebl_dwarf_to_regno): Remove redundant NULL tests
+	on parameters declared with __nonnull_attribute__.
+	* eblinitreg.c (ebl_frame_nregs): Likewise.
+	* eblnormalizepc.c (ebl_normalize_pc): Likewise.
+	* eblunwind.c (ebl_unwind): Likewise.
+
 2015-09-04  Chih-Hung Hsieh  <chh@google.com>
 
 	* eblopenbackend.c (ebl_openbackend_machine): Replace K&R function
diff --git a/libebl/ebldwarftoregno.c b/libebl/ebldwarftoregno.c
index 8fb8540..c664496 100644
--- a/libebl/ebldwarftoregno.c
+++ b/libebl/ebldwarftoregno.c
@@ -35,7 +35,6 @@
 bool
 ebl_dwarf_to_regno (Ebl *ebl, unsigned *regno)
 {
-  if (ebl == NULL)
-    return false;
+  /* ebl is declared NN */
   return ebl->dwarf_to_regno == NULL ? true : ebl->dwarf_to_regno (ebl, regno);
 }
diff --git a/libebl/eblinitreg.c b/libebl/eblinitreg.c
index 5729b3c..8a3fb18 100644
--- a/libebl/eblinitreg.c
+++ b/libebl/eblinitreg.c
@@ -47,7 +47,8 @@ ebl_set_initial_registers_tid (Ebl *ebl, pid_t tid,
 size_t
 ebl_frame_nregs (Ebl *ebl)
 {
-  return ebl == NULL ? 0 : ebl->frame_nregs;
+  /* ebl is declared NN */
+  return ebl->frame_nregs;
 }
 
 GElf_Addr
diff --git a/libebl/eblnormalizepc.c b/libebl/eblnormalizepc.c
index a5fea77..1629353 100644
--- a/libebl/eblnormalizepc.c
+++ b/libebl/eblnormalizepc.c
@@ -35,6 +35,7 @@
 void
 ebl_normalize_pc (Ebl *ebl, Dwarf_Addr *pc)
 {
-  if (ebl != NULL && ebl->normalize_pc != NULL)
+  /* ebl is declared NN */
+  if (ebl->normalize_pc != NULL)
     ebl->normalize_pc (ebl, pc);
 }
diff --git a/libebl/eblunwind.c b/libebl/eblunwind.c
index 1251c1b..2970d03 100644
--- a/libebl/eblunwind.c
+++ b/libebl/eblunwind.c
@@ -37,7 +37,8 @@ ebl_unwind (Ebl *ebl, Dwarf_Addr pc, ebl_tid_registers_t *setfunc,
 	    ebl_tid_registers_get_t *getfunc, ebl_pid_memory_read_t *readfunc,
 	    void *arg, bool *signal_framep)
 {
-  if (ebl == NULL || ebl->unwind == NULL)
+  /* ebl is declared NN */
+  if (ebl->unwind == NULL)
     return false;
   return ebl->unwind (ebl, pc, setfunc, getfunc, readfunc, arg, signal_framep);
 }
-- 
2.4.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]