Mark, your updated patch works fine and compiles cleanly with clang. Thanks for the update.
On Thu, Sep 10, 2015 at 4:55 AM, Mark Wielaard <m...@redhat.com> wrote: > 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 <c...@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 >
