[PATCH]: Re: Regression with strn-stuff

[Pedro Alves <pedro_alves at portugalmail dot pt>]

Hi all,

Hans-Peter Nilsson escreveu:
> > Date: Sun, 17 Sep 2006 19:57:24 +0100
> > From: Pedro Alves <pedro_alves@portugalmail.pt>
> >     
> > In my original:
> > #define const_strcmp(DST, ORG) \
> >    strncmp (DST, ORG "", sizeof (ORG) - 1)
> >     
>
> Oh!  I didn't notice, but then again I didn't look at your
> original submission very hard (just enough for a "that sounds
> good").
>
>   
Humm, I think everyone missed the value of my other macro.
The check_strncmp below:

#define STR_STRLEN_ASSERT(COND, MSG) \
  sizeof (struct { char MSG[ !(COND) ? -1 : 1 ]; })

#define check_strncmp(DST, SRC, N) \
  ( STR_STRLEN_ASSERT ((sizeof (SRC "") - 1) == (N), \
            check_strncmp_length_mismatch), \
  strncmp (DST, SRC, N) )

This is working on top of the fact that comma operator in an expression,
returns the value of the right member.
In the A = (B, C) case, when B has no side effects,
is equivalent to A = C. I used this to put the assert into B.

This macro is useful in cases like in:

bfd/archive.c:

bfd_boolean
bfd_slurp_armap (bfd *abfd)
{
 char nextname[17];
 int i = bfd_bread (nextname, 16, abfd);

 if (i == 0)
   return TRUE;
 if (i != 16)
   return FALSE;

 if (bfd_seek (abfd, (file_ptr) -16, SEEK_CUR) != 0)
   return FALSE;

 if (check_strncmp (nextname, "__.SYMDEF      ", 16)
     || check_strncmp (nextname, "__.SYMDEF/      ", 16)) /* Old Linux 
archives.  */
   return do_slurp_bsd_armap (abfd);
 else if (check_strncmp (nextname, "/               ", 16))
   return do_slurp_coff_armap (abfd);
 else if (check_strncmp (nextname, "/SYM64/         ", 16))
   {
     /* 64bit ELF (Irix 6) archive.  */
#ifdef BFD64
     extern bfd_boolean bfd_elf64_archive_slurp_armap (bfd *);
     return bfd_elf64_archive_slurp_armap (abfd);
#else
     bfd_set_error (bfd_error_wrong_format);
     return FALSE;
#endif
   }

 bfd_has_map (abfd) = FALSE;
 return TRUE;
}

(I'm not at all familiar with this code, I picked this out randomly.)

Did you notice the bug I introduced above? The compiler does.

Hint, count the len of "__.SYMDEF      " at:
 if (check_strncmp (nextname, "__.SYMDEF      ", 16)

In these cases, the explicit string len is important information you
want available (*), and replacing the strncmp with CONST_STRNEQ
might be hiding bugs, like in:
 if (CONST_STRNEQ (nextname, "__.SYMDEF      ")
Here we would only be comparing 15 chars, while we should be comparing 16.

(Note, this bug in *not* in the files, I added it here.)

Here is an updated version, that passes gcc's -Wall -Werror too.

#define CHECK_STRNCMP(STR1, STR2, N) \
   ( ((void)sizeof (struct { \
       char check_strncmp_length_mismatch[ \
       ((sizeof (STR2) - 1) == (N)) ? 1 : -1 ]; })), \
   strncmp (STR1, STR2 "", N) )

(*) - One could argue that this constant should be placed in a define.

-----------------------

> > The double double-quotes ("") were there to check if the parameter
> > was a compile time const string, since it is legal in C to write a 
> > string this way:
> > ("string part 1" "string part 2").
> >     
> > Isn't the double quotes version OK?
> >     
>
> With a comment why it's there, this sounds to me like the Right Thing.
>
>   
Attached is a patch to do just that. I only tested this on arm-wince-pe,
but I see no regressions there. Sorry I didn't test more, but I am
out-of-office for the week, on a quite limited machine.
I see no other portable way to check if a string is compile time constant,
but I would love to know other ways.
If this turns out to be too limiting, we will indeed have to turn
to __builtin_constant_p for gcc...

Please (test,) review, and commit.

Cheers,
Pedro Alves

---
bfd/ChangeLog

2006-09-17  Pedro Alves  <pedro_alves@portugalmail.pt>


   * bfd-in.h (CONST_STRNEQ): Make it error out if STR2 is not a 
compile time constant string.
   (CONST_STRNCPY): Likewise.
   * bfd-in2.h: Regenerate.

Index: bfd-in.h
===================================================================
RCS file: /cvs/src/src/bfd/bfd-in.h,v
retrieving revision 1.117
diff -u -p -r1.117 bfd-in.h
--- bfd-in.h	16 Sep 2006 18:12:13 -0000	1.117
+++ bfd-in.h	17 Sep 2006 21:59:33 -0000
@@ -57,9 +57,13 @@ extern "C" {
    the arguments to the strncmp() macro.  Hence this alternative
    definition of strncmp is provided here.
    
-   Note - these macros do NOT work if STR2 is not a constant string.  */
-#define CONST_STRNEQ(STR1,STR2) (strncmp ((STR1), (STR2), sizeof (STR2) - 1) == 0)
-#define CONST_STRNCPY(STR1,STR2) strncpy ((STR1), (STR2), sizeof (STR2) - 1)
+   Note - the double double-quotes ("") are there to ensure that
+   STR2 is a compile-time constant string. 
+   This works, because in C it is possible to define a string
+   in parts like in:
+   const char* str = "str part 1" "str part 2";  */
+#define CONST_STRNEQ(STR1,STR2) (strncmp (STR1, STR2 "", sizeof (STR2) - 1) == 0)
+#define CONST_STRNCPY(STR1,STR2) strncpy (STR1, STR2 "", sizeof (STR2) - 1)
 
 
 /* The word size used by BFD on the host.  This may be 64 with a 32