Patch: Problem with type safety and the "sentinel" attribute

[Kaveh R. Ghazi]

 > Hi!
 > There has been some discussion with the same subject on the gcc list,
 > but I am moving this to gcc-patches now, since I've got a patch now,
 > which you'll find at the end of the mail.

Thanks for the patch, I have some feedback I'd like to offer.  Note I
can't approve your work for installation, but since I contributed the
sentinel feature I have some background on this issue.


 > So the provided patch adds the null_terminated attribute as suggested,
 > and works as I would expect it to work.

Sorry, I don't like the name "null_terminated".  It implies two things
that aren't necessary.  The first is that NULL must be the terminator,
and second that it must appear as the terminating i.e. "last"
argument.  Neither is necessarily true and we shouldn't box ourselves
in with the name.  Look at the design I wrote for attribute sentinel:
http://gcc.gnu.org/ml/gcc/2004-08/msg01068.html

In theory it takes two optional arguments, position and value (which
would also specify type.)  I only implemented the position, but
nothing stops someone from implementing value/type which has been
asked for in the past and most recently by Tim in this thread.  So I'd
like to come up with a name that is neutral on the magic value and
position.  Maybe "named_sentinel" ?  I'm open to other ideas.

Note, you don't have to necessarily implement the value/type thing,
but your design should leave the door open to later backwards
compatible improvements.

For implementation, my suggestion is to copy all of the sentinel code
as your starting point.  Where the sentinel code skips over the named
arguments, you could simply stop one parameter earlier.  Or reuse the
sentinel code by adding a bool parameter to check_function_sentinel
which says whether to stop on the last named argument before starting
to search for the sentinel.


 > There is only one thing which I haven't explicitely implemented: a
 > varargs function with this prototype:
 > 
 > extern void ifoo (const char *name,
 >                   int i, ...) __attribute__ ((null_terminated));
 > 
 > should not allow NULL termination within the "i" parameter (which has
 > the wrong type to be suitable for NULL termination):
 > 
 >   ifoo ("warn", NULL);                /* <- should warn */
 >   ifoo ("do not warn", 0, NULL);      /* <- should not warn */
 > 
 > Although I wrote no code to handle this case explicitely, gcc does
 > emit the right warning anyway. So essentially even this case works,
 > but I don't know why it does. But I admit I don't fully understand
 > those parts of the check which I simply copypasted from the
 > sentinel code.

If there are any parts of the code you don't understand, feel free to
ask me specific questions (and cc this list.)

You definitely want to check corner cases like the above and your
patch should include a rigorous testcase.  See the sentinel testcase
gcc.dg/format/sentinel-1.c for ideas.

Hope this helps.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu