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

[Chih-Hung Hsieh <chh at google dot com>]

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 <mjw@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 <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
>

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 <mjw@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 <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