On Wed, 12 Mar 2008, chromatic wrote:
> On Wednesday 12 March 2008 15:07:08 Andy Lester wrote:
>
> > On Mar 12, 2008, at 4:50 PM, Andy Dougherty via RT wrote:
> > > The problem is twofold:
> >
> > I'm not sure what you're saying is a problem. Are you saying that the
> > use of __attribute__nonnull__ is a problem? Or just that we don't
> > have it set correctly everywhere?
>
> I believe he means the latter. If GCC uses these hints during optimization
> and Parrot crashes in places where it tries to dereference NULL pointers (and
> in fact this is what happens), then the annotations are wrong for the state
> of the code.
I actually meant both, but with different strengths of conviction.
Incorrect annotations are, obviously and noncontroversially, a problem. No
big deal here. (Incidentally -- anyone with a relatively recent gcc can
probably reproduce this. I used gcc-4.1.0 on SPARC. Just build parrot
with something like
perl Configure.pl --debugging=0 --optimize=-O4
I don't know what precise optimization level is required. -O4 did it for
me.)
Second, even if they are correct, I am unconvinced they are, on balance,
useful. I think a simple assert() in the code might be preferable.
Others might reasonably disagree.
To give a specific example: Consider this function defined on line 763 in
src/headers.c:
static int
sweep_cb_buf(PARROT_INTERP, ARGMOD(Small_Object_Pool *pool),
SHIM(int flag), SHIM(void *arg))
Yesterday, I was trying to debug a problem where pool was NULL. I tried
adding various checks to the function, unaware that 712 lines above,
on line 51, pool was listed as nonnull. Gcc kept optimizing away my
checks, so parrot kept crashing and I was deceived into thinking the
problem wasn't a null pool. I finally remembered that I had done this
dance once before and that I first had to remove the nonnull decoration
before I could get back to debugging.
(Incidentally, I also had to remove the SHIM around *arg because it's
not a SHIM with the --gc=libc option, but that was just a separate
irritation.)
So what did __attribute__nonnull actually gain me here? It is technically
correct. pool should not be null here. However, it didn't seem to help
in any way. Without it, parrot crashed. With it, parrot crashed in the
same place, but I couldn't put in any if (!pool) checks without hunting
down and eliminating the nonnull attribute. And since it is defined some
distance away, it might not always be obvious whether it's there or not.
In short, I don't see __attribute__nonnull as a seat belt. Instead it's
a guarantee we're making to the compiler. It's more like a sign saying
it's safe to take off the seat belts: "Hey, I don't need any protection
here -- this pointer is guaranteed not to be NULL."
Anyway, that's my current opinion.
--
Andy Dougherty [EMAIL PROTECTED]
