[PATCH]: Re: Regression with strn-stuff

[Pedro Alves]

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.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bfd-in.h.diff
URL: <https://sourceware.org/pipermail/binutils/attachments/20060917/3b7d4117/attachment.ksh>