Couple of bugs

Steve Grubb sgrubb at redhat.com
Wed Feb 25 16:37:36 CET 2009


On Wednesday 25 February 2009 01:46:36 am Marc Lehmann wrote:
> On Mon, Feb 23, 2009 at 09:22:28AM -0500, Steve Grubb <sgrubb at redhat.com> 
wrote:
> > > C has well-defined behaviour in this case. Please read the section
> > > about "warnings" in the manual. In short: we cannot care for each and
> > > every weird compiler warning implemented by your compiler - if you
> > > enable warnings for your compiler that flag valid code, then you have
> > > to deal with it yourself.
> >
> > But why have the warning at all when its easy to fix? :)
>
> Because compilers have all sorts of warnings. 

In my experience, they each help to write better code. :)


> Implementing workarounds for each and every compiler will greatly reduce
> maintainability of the source code.

Let's take a look in glibc, assert.h does it the way I mention as does argp.h 
and features.h.


> Warnings are also often more or less bogus (for example, the aliasing
> warnings in some versions of gcc).

Those warnings are telling you that the code would run faster if they were 
fixed. But that's not really the warnings I see. I checked using this:

-W -Wall -Wshadow -Wformat -Wfloat-equal -Wundef


> Please read the relevant section about compiler warnings in the docs.
>
> Just FYI: I do not get a warning about that with my compiler (which is
> gcc-4.3.2).

Are you compiling with -Wundef? 


> > In any event, I did some more digging and found another very minor issue.
> > In ev_poll.c at line 96, we have this code:
>
> You probably mean ev_epoll.c?

yes, sorry.


> > nev is not updated between the two. So, the code at line 96 would never
> > execute.
>
> There is lots of code in libev that will never execute under spoecific
> circumstances (for example, all asserts).

So, what happens if I define NDEBUG to get rid of the asserts? Isn't it bad to 
make function calls inside asserts since they get deleted? For example:

assert (("libev: only socket fds supported in this configuration"
, ioctlsocket (anfd->handle, FIONREAD, &arg) == 0));

or

assert (("libev: active index mismatch in heap", ev_active (ANHE_w (heap [i])) 
== i));

Hmm....if you run "strings" on the resulting program and grep for "ev", you 
will see that each of these are stored as text in the program and the assert 
is not executed:

("libev: watcher has invalid priority", (((W)w)->priority - -2) >= 0 && 
(((W)w)->priority - -2) < (+2 - -2 + 1))
("libev: pending watcher not on pending queue", ((loop)->pendings) 
[(((W)w)->priority - -2)][w->pending - 1].w == w)
("libev: active index mismatch in heap", ((W)((heap [i]).w))->active == i)
("libev: heap at cache mismatch", (heap [i]).at == ((WT)((heap [i]).w))->at)

Was that intentional?


> That's not an issue, as the code will work correctly, the code is simply an
> algorithmic hint.

Hint? Its dead code. If nev == 0, it returns. That's it. Checking for nev == 0 
again is wasting clock cycles, but it could also be an indication that some 
code that updated nev got deleted in the past. I can't tell what the 
intentions were, so I'm asking.


>Please do read the valgrind section in the manual.

My project's requirements are clean valgrind run no matter who's code its in.


>Your patch is architecture dependend and will not work when 
>destructors are not working (which is not uncommon).

So, which version of gcc are they working reliably in? I did not see any arch 
specific warnings or hints in the gcc docs. We've been using destructors in 
security sensitive libraries since gcc 3.4 on at least 6 different arches.

I did some more looking around. In ev.c is a function, pipecb. What if a 
SIGCHLD is delivered when the read is called? Same issue in evpipe_write for 
the writes.

Thanks,
-Steve



More information about the libev mailing list