[systemd-devel] [PATCH 2/5] libsystemd:terminal :fix uninitialized warning

[Tom Gundersen]

On Mon, Nov 3, 2014 at 1:46 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> On Mon, Nov 3, 2014 at 1:44 PM, Zbigniew Jędrzejewski-Szmek
> <zbyszek at in.waw.pl> wrote:
>> On Mon, Nov 03, 2014 at 01:41:20PM +0100, Lennart Poettering wrote:
>>> On Mon, 03.11.14 13:12, David Herrmann (dh.herrmann at gmail.com) wrote:
>>>
>>> > Hi
>>> >
>>> > On Thu, Oct 16, 2014 at 11:43 PM,  <philippedeswert at gmail.com> wrote:
>>> > > From: Philippe De Swert <philippedeswert at gmail.com>
>>> > >
>>> > > Remove the following warning during the compilation:
>>> > > src/libsystemd-terminal/grdev-drm.c: In function 'grdrm_card_hotplug':
>>> > > src/libsystemd-terminal/grdev-drm.c:1087:45: warning: 'fb' may be used uninitialized in this function [-Wmaybe-uninitialized]
>>> > > src/libsystemd-terminal/grdev-drm.c:1035:19: note: 'fb' was declared here
>>> > > ---
>>> > >  src/libsystemd-terminal/grdev-drm.c | 2 +-
>>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>>> > >
>>> > > diff --git a/src/libsystemd-terminal/grdev-drm.c b/src/libsystemd-terminal/grdev-drm.c
>>> > > index dba6db2..415755e 100644
>>> > > --- a/src/libsystemd-terminal/grdev-drm.c
>>> > > +++ b/src/libsystemd-terminal/grdev-drm.c
>>> > > @@ -1032,7 +1032,7 @@ error:
>>> > >
>>> > >  static void grdrm_crtc_expose(grdrm_crtc *crtc) {
>>> > >          grdrm_pipe *pipe;
>>> > > -        grdrm_fb *fb;
>>> > > +        grdrm_fb *fb  = NULL;
>>> >
>>> > Ewww, this is not nice. It does fix the warning, indeed, but the
>>> > underlying problem is more generic. Lets look at this:
>>> >
>>> > int some_function(void **out) {
>>> > ...
>>> >         r = ioctl(...);
>>> >         if (r < 0)
>>> >                 return -errno;
>>> > ...
>>> > }
>>> >
>>> > gcc has no guarantee that "errno" is <0 if r is <0. Therefore,
>>> > whenever it inlines those functions (-O2 etc.), it will spew a warning
>>> > that "out" might be uninitialized if r<0 but errno==0. With -O0 gcc
>>> > doesn't complain as it probably does not optimize across functions.
>>> > However, with -O2 I get those warnings for "static" functions all the
>>> > time.
>>> >
>>> > Not sure what to do here. I dislike initializing the pointer to NULL
>>> > as it might hide other real warnings. I'd prefer something like:
>>> >
>>> > r = -SANE_ERRNO;
>>> >
>>> > with:
>>> >
>>> > #define SANE_ERRNO (abs(errno) ? : EMAGIC)
>>> >
>>> > ...not sure what we do in other places, though. Lennart? Tom?
>>>
>>> So far we went the simple way out and merged patches like the original
>>> one you are replying to.
>>>
>>> Adding a call like this would make sane to me though:
>>>
>>> static inline negative_errno(void) {
>>>        return _likely_(errno > 0) ? -errno : -EINVAL;
>>> }
>>>
>>> Which is then invoked as:
>>>
>>>       return negative_errno();
>>>
>>> or so...
>> I like the assert more, because having errno <= 0 would be a signficant bug
>> in the system, not something that we want to ever hide. So basically this
>> is a way to tell the compiler what we already know, and assert is better.
>
> Indeed. So how about:
>
> static inline int negative_errno(void) {
>         assert_return(errno > 0, -EINVAL);
>         return -errno;
> }

Looks good to me.

-t