[PATCH] Make the compiler do the math 2.

[PATCH] Make the compiler do the math 2.

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

Hi all,

Same idea as my previous post (1), but applied to a file that
has just until very recently a str/strlen pair bug, fixed by H.J.LU.

Please review and commit.

(1) http://sourceware.org/ml/binutils/2006-09/msg00076.html

Cheers,
Pedro Alves

---

bfd/ChangeLog

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

	* elf.c (_bfd_elf_make_section_from_shdr): New macro DSECT.
	(_bfd_elf_make_section_from_shdr, debug_sections): Use DSECT.


[-- Attachment #2: elf.c.diff --]
[-- Type: text/plain, Size: 1519 bytes --]

--- elf.c.org	2006-09-11 21:17:02.000000000 +0100
+++ elf.c	2006-09-11 21:25:18.000000000 +0100
@@ -816,22 +816,24 @@ _bfd_elf_make_section_from_shdr (bfd *ab
 	  int len;
 	} debug_sections [] =
 	{
-	  { "debug",		 5  },	/* 'd' */
-	  { NULL,		 0  },	/* 'e' */
-	  { NULL,		 0  },	/* 'f' */
-	  { "gnu.linkonce.wi.", 16 },	/* 'g' */
-	  { NULL,		 0  },	/* 'h' */
-	  { NULL,		 0  },	/* 'i' */
-	  { NULL,		 0  },	/* 'j' */
-	  { NULL,		 0  },	/* 'k' */
-	  { "line",		 4  },	/* 'l' */
-	  { NULL,		 0  },	/* 'm' */
-	  { NULL,		 0  },	/* 'n' */
-	  { NULL,		 0  },	/* 'o' */
-	  { NULL,		 0  },	/* 'p' */
-	  { NULL,		 0  },	/* 'q' */
-	  { NULL,		 0  },	/* 'r' */
-	  { "stab",		 4  }	/* 's' */
+#define DSECT(STR) { (STR), ((STR) ? sizeof (STR) - 1 : 0) }
+	  DSECT("debug"),               /* 'd' */
+	  DSECT(0),                     /* 'e' */
+	  DSECT(0),                     /* 'f' */
+	  DSECT("gnu.linkonce.wi."),    /* 'g' */
+	  DSECT(NULL),                  /* 'h' */
+	  DSECT(NULL),                  /* 'i' */
+	  DSECT(NULL),                  /* 'j' */
+	  DSECT(NULL),                  /* 'k' */
+	  DSECT("line"),                /* 'l' */
+	  DSECT(NULL),                  /* 'm' */
+	  DSECT(NULL),                  /* 'n' */
+	  DSECT(NULL),                  /* 'o' */
+	  DSECT(NULL),                  /* 'p' */
+	  DSECT(NULL),                  /* 'q' */
+	  DSECT(NULL),                  /* 'r' */
+	  DSECT("stab")                 /* 's' */
+#undef DSECT
 	};
       
       if (name [0] == '.')

Re: [PATCH] Make the compiler do the math 2.

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

Same patch with the formatting inline with GNU standards.

Sorry again for the noise.

Please review and commit.

Cheers,
Pedro Alves

---

bfd/ChangeLog

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

     * elf.c (_bfd_elf_make_section_from_shdr): New macro DSECT.
     (_bfd_elf_make_section_from_shdr, debug_sections): Use DSECT.

[-- Attachment #2: elf.c.diff --]
[-- Type: text/plain, Size: 1535 bytes --]

--- elf.c.org	2006-09-12 07:12:05.000000000 -0400
+++ elf.c	2006-09-12 07:17:57.000000000 -0400
@@ -816,22 +816,24 @@ _bfd_elf_make_section_from_shdr (bfd *ab
 	  int len;
 	} debug_sections [] =
 	{
-	  { "debug",		 5  },	/* 'd' */
-	  { NULL,		 0  },	/* 'e' */
-	  { NULL,		 0  },	/* 'f' */
-	  { "gnu.linkonce.wi.", 16 },	/* 'g' */
-	  { NULL,		 0  },	/* 'h' */
-	  { NULL,		 0  },	/* 'i' */
-	  { NULL,		 0  },	/* 'j' */
-	  { NULL,		 0  },	/* 'k' */
-	  { "line",		 4  },	/* 'l' */
-	  { NULL,		 0  },	/* 'm' */
-	  { NULL,		 0  },	/* 'n' */
-	  { NULL,		 0  },	/* 'o' */
-	  { NULL,		 0  },	/* 'p' */
-	  { NULL,		 0  },	/* 'q' */
-	  { NULL,		 0  },	/* 'r' */
-	  { "stab",		 4  }	/* 's' */
+#define DSECT(STR) { (STR), ((STR) ? sizeof (STR) - 1 : 0) }
+	  DSECT ("debug"),               /* 'd' */
+	  DSECT (NULL),                  /* 'e' */
+	  DSECT (NULL),                  /* 'f' */
+	  DSECT ("gnu.linkonce.wi."),    /* 'g' */
+	  DSECT (NULL),                  /* 'h' */
+	  DSECT (NULL),                  /* 'i' */
+	  DSECT (NULL),                  /* 'j' */
+	  DSECT (NULL),                  /* 'k' */
+	  DSECT ("line"),                /* 'l' */
+	  DSECT (NULL),                  /* 'm' */
+	  DSECT (NULL),                  /* 'n' */
+	  DSECT (NULL),                  /* 'o' */
+	  DSECT (NULL),                  /* 'p' */
+	  DSECT (NULL),                  /* 'q' */
+	  DSECT (NULL),                  /* 'r' */
+	  DSECT ("stab")                 /* 's' */
+#undef DSECT
 	};
       
       if (name [0] == '.')

Re: [PATCH] Make the compiler do the math 2.

[Nick Clifton]

Hi Pedro,

> bfd/ChangeLog
> 2006-09-12  Pedro Alves  <pedro_alves@portugalmail.pt>
> 
>     * elf.c (_bfd_elf_make_section_from_shdr): New macro DSECT.
>     (_bfd_elf_make_section_from_shdr, debug_sections): Use DSECT.

Thanks for working on this.

I have taken your patch, renamed the macro to STRING_COMMA_LEN, and 
added a second macro CONST_STRNEQ, but them in bfd.h and then used them 
everywhere in the bfd code where I could apply them.  I have tested as 
thoroughly as I can, and I think that there are no regressions although 
of course I will fix anything that does turn up.

Cheers
   Nick

bfd/ChangeLog
2006-09-16  Nick Clifton  <nickc@redhat.com>
	    Pedro Alves  <pedro_alves@portugalmail.pt>

	* bfd-in.h (STRING_AND_COMMA): New macro.  Takes one constant
	string as its argument and emits the string followed by a comma
	and then the length of the string.
	(CONST_STRNEQ): New macro.  Checks to see if a variable string
	has a constant string as its initial characters.
	(CONST_STRNCPY): New macro.  Copies a constant string to the start
	of a variable string.
	* bfd-in2.h: Regenerate.
	* archive.c: Make use of the new macros.
	* archive64.c: Likewise.
	* bfd.c: Likewise.
	* coff-ppc.c: Likewise.
	* coff-stgo32.c: Likewise.
	* coffcode.h: Likewise.
	* cofflink.c: Likewise.
	* cpu-i960.c: Likewise.
	* dwarf2.c: Likewise.
	* ecoff.c: Likewise.
	* elf-m10300.c: Likewise.
	* elf.c: Likewise.
	* elf32-arm.c: Likewise.
	* elf32-bfin.c: Likewise.
	* elf32-cris.c: Likewise.
	* elf32-hppa.c: Likewise.
	* elf32-i370.c: Likewise.
	* elf32-i386.c: Likewise.
	* elf32-iq2000.c: Likewise.
	* elf32-m32r.c: Likewise.
	* elf32-m68hc11.c: Likewise.
	* elf32-m68hc12.c: Likewise.
	* elf32-m68k.c: Likewise.
	* elf32-mcore.c: Likewise.
	* elf32-ppc.c: Likewise.
	* elf32-s390.c: Likewise.
	* elf32-sh-symbian.c: Likewise.
	* elf32-sh.c: Likewise.
	* elf32-sh64.c: Likewise.
	* elf32-v850.c: Likewise.
	* elf32-vax.c: Likewise.
	* elf32-xtensa.c: Likewise.
	* elf64-alpha.c: Likewise.
	* elf64-hppa.c: Likewise.
	* elf64-mmix.c: Likewise.
	* elf64-ppc.c: Likewise.
	* elf64-s390.c: Likewise.
	* elf64-sh64.c: Likewise.
	* elf64-x86-64.c: Likewise.
	* elflink.c: Likewise.
	* elfxx-ia64.c: Likewise.
	* elfxx-mips.c: Likewise.
	* elfxx-sparc.c: Likewise.
	* hpux-core.c: Likewise.
	* i386linux.c: Likewise.
	* ieee.c: Likewise.
	* libpei.h: Likewise.
	* linker.c: Likewise.
	* m68klinux.c: Likewise.
	* mmo.c: Likewise.
	* nlmcode.h: Likewise.
	* osf-core.c: Likewise.
	* pef.c: Likewise.
	* som.c: Likewise.
	* sparclinux.c: Likewise.
	* vms-hdr.c: Likewise.

Regression with strn-stuff (was: Re: [PATCH] Make the compiler do the math 2.)

[Hans-Peter Nilsson]

> Date: Sat, 16 Sep 2006 19:16:00 +0100
> From: Nick Clifton <nickc@redhat.com>

> I have tested as 
> thoroughly as I can, and I think that there are no regressions although 
> of course I will fix anything that does turn up.

These changes were logged together with this regression for
--target=cris-elf:

...
Running /h/hp/binutils/cvs_latest/src/ld/testsuite/ld-elf/elf.exp ...
FAIL: ld-elf/stab
...

I looked briefly but can't see the presumed typo.  However, I
spotted a different one:

Index: cofflink.c
===================================================================
RCS file: /cvs/src/src/bfd/cofflink.c,v
retrieving revision 1.60
retrieving revision 1.61
diff -u -p -r1.60 -r1.61
--- cofflink.c	5 Jul 2006 10:21:39 -0000	1.60
+++ cofflink.c	16 Sep 2006 18:12:13 -0000	1.61
...
@@ -583,7 +583,7 @@ coff_link_add_symbols (bfd *abfd,
 	  asection *stab;
 	  
 	  for (stab = abfd->sections; stab; stab = stab->next)
-	    if (strncmp (".stab", stab->name, 5) == 0
+	    if (CONST_STRNEQ (".stab", stab->name)
 		&& (!stab->name[5]
 		    || (stab->name[5] == '.' && ISDIGIT (stab->name[6]))))
 	    {


Gotta find a way to automatically check the const-stringness of
the second operand.  Use __builtin_constant_p in a checking
macro enabled for gcc?

brgds, H-P

Re: Regression with strn-stuff

[Nick Clifton]

Hi Hans-Peter,

> Running /h/hp/binutils/cvs_latest/src/ld/testsuite/ld-elf/elf.exp ...
> FAIL: ld-elf/stab

Yes I noticed this failure turning up recently with a number of ports, 
but when I checked I found that it was not connected to the CONST_STRNEQ 
patch but to something else.  (I have not had the time to find out what 
yet).

> -	    if (strncmp (".stab", stab->name, 5) == 0
> +	    if (CONST_STRNEQ (".stab", stab->name)

Oops - thanks - I have checked in a patch to correct this.

> Gotta find a way to automatically check the const-stringness of
> the second operand.  Use __builtin_constant_p in a checking
> macro enabled for gcc?

Is there such a function ?  I did not know, but if it does exist then 
yes, using it would be a good idea.

Cheers
   Nick

Re: Regression with strn-stuff

[Pedro Alves]

Nick Clifton escreveu:
> Hans-Peter wrote:
>
>> Gotta find a way to automatically check the const-stringness of
>> the second operand.  Use __builtin_constant_p in a checking
>> macro enabled for gcc?
>
> Is there such a function ?  I did not know, but if it does exist then 
> yes, using it would be a good idea.
>

In my original:
#define const_strcmp(DST, ORG) \
   strncmp (DST, ORG "", sizeof (ORG) - 1)

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").

I had played with __builtin_constant_p too, but since it was gcc 
specific I dropped it.
Isn't the double quotes version OK?

The double double-quotes ) version only doesn't work when you pass
CONST_STRNEQ a string inside parentheses, like in:

#define MY_STRING ("a string")
CONST_STRNEQ (a_str, MY_STRING);

Since it would expand to:
... ("a string") "" ...
, which doesn't work. But the parenthesis in this cases add no value,
so then can be safely removed.

Cheers,
Pedro Alves

Re: Regression with strn-stuff

[Hans-Peter Nilsson]

> Date: Sun, 17 Sep 2006 15:40:53 +0100
> From: Nick Clifton <nickc@redhat.com>

> > Running /h/hp/binutils/cvs_latest/src/ld/testsuite/ld-elf/elf.exp ...
> > FAIL: ld-elf/stab
> 
> Yes I noticed this failure turning up recently with a number of ports, 
> but when I checked I found that it was not connected to the CONST_STRNEQ 
> patch but to something else.

Uh hmm, did you actually "find that it was not connected" (by
e.g. reverting the patch and re-test) or did you perhaps just do
what I did; stare at the patch but did not find the bug? ;-}

I'd guess the latter, because of the following I just spotted.
Though I did not revert, build and test, I looked a few more
times, because it *had* to be connected to those changes.  It's
not that I distrust you personally (hey, *everybody* lies to me 8-)
but as I said there were no other changes in the cvs update
output in my build log.

Index: elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.353
retrieving revision 1.354
diff -u -p -r1.353 -r1.354
--- elf.c	14 Sep 2006 12:11:33 -0000	1.353
+++ elf.c	16 Sep 2006 18:12:13 -0000	1.354
 static const struct bfd_elf_special_section special_sections_s[] =
 {
-  { ".shstrtab",       9,  0, SHT_STRTAB,   0 },
-  { ".strtab",         7,  0, SHT_STRTAB,   0 },
-  { ".symtab",         7,  0, SHT_SYMTAB,   0 },
-  { ".stabstr",        5,  3, SHT_STRTAB,   0 },
-  { NULL,              0,  0, 0,            0 }
+  { STRING_COMMA_LEN (".shstrtab"), 0, SHT_STRTAB, 0 },
+  { STRING_COMMA_LEN (".strtab"),   0, SHT_STRTAB, 0 },
+  { STRING_COMMA_LEN (".symtab"),   0, SHT_SYMTAB, 0 },
+  { STRING_COMMA_LEN (".stabstr"),  3, SHT_STRTAB, 0 },
+  { NULL,                       0,  0, 0,          0 }
 };
 
I committed the following as obvious (when you consider the new
comment and the usage of the table) after build+test for cris-elf.

bfd:
	* elf.c (special_sections_s): Revert last STRING_COMMA_LEN change
	for .stabstr entry, explain why.

Index: elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.354
diff -u -p -r1.354 elf.c
--- elf.c	16 Sep 2006 18:12:13 -0000	1.354
+++ elf.c	17 Sep 2006 18:50:00 -0000
@@ -2374,7 +2374,9 @@ static const struct bfd_elf_special_sect
   { STRING_COMMA_LEN (".shstrtab"), 0, SHT_STRTAB, 0 },
   { STRING_COMMA_LEN (".strtab"),   0, SHT_STRTAB, 0 },
   { STRING_COMMA_LEN (".symtab"),   0, SHT_SYMTAB, 0 },
-  { STRING_COMMA_LEN (".stabstr"),  3, SHT_STRTAB, 0 },
+  /* See struct bfd_elf_special_section declaration for the semantics of
+     this special case where .prefix_length != strlen (.prefix).  */
+  { ".stabstr",			5,  3, SHT_STRTAB, 0 },
   { NULL,                       0,  0, 0,          0 }
 };
 

brgds, H-P

Re: Regression with strn-stuff

[Hans-Peter Nilsson]

> 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").

> 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.

brgds, H-P

[PATCH]: Re: Regression with strn-stuff

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]

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.


[-- Attachment #2: bfd-in.h.diff --]
[-- Type: text/plain, Size: 1127 bytes --]

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

Re: [PATCH]: Re: Regression with strn-stuff

[Pedro Alves]

>>> 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?
>>>     
>>
>>>>   
> Attached is a patch to do just that.

(...)

> 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.
> 

I withdraw this patch. I found some places where this was too
limiting. It turns out that in some places the ()'s around
the passed in string literal were really needed,
making ("a string") "" uncompilable.
I don't think there is any way in C to implement the
REMOVE_PARENS macro below, is there?

#define REMOVE_PARENS(S) (?)
REMOVE_PARENS(("a string")) => "A STRING"

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

Re: [PATCH] Make the compiler do the math 2.

[Andreas Schwab]

Nick Clifton <nickc@redhat.com> writes:

> Hi Pedro,
>
>> bfd/ChangeLog
>> 2006-09-12  Pedro Alves  <pedro_alves@portugalmail.pt>
>>
>>     * elf.c (_bfd_elf_make_section_from_shdr): New macro DSECT.
>>     (_bfd_elf_make_section_from_shdr, debug_sections): Use DSECT.
>
> Thanks for working on this.
>
> I have taken your patch, renamed the macro to STRING_COMMA_LEN, and added
> a second macro CONST_STRNEQ, but them in bfd.h and then used them
> everywhere in the bfd code where I could apply them.  I have tested as
> thoroughly as I can, and I think that there are no regressions although of
> course I will fix anything that does turn up.

You can't use STRING_COMMA_LEN as part of a function call because it may
be a macro:

../../bfd/elflink.c:10069:47: error: macro "memcpy" requires 3 arguments, but only 2 given

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Regression with strn-stuff

[Nick Clifton]

Hi Hans-Peter,

> Uh hmm, did you actually "find that it was not connected" (by
> e.g. reverting the patch and re-test)

Yup that's what I did.

> Though I did not revert, build and test, I looked a few more
> times, because it *had* to be connected to those changes.  It's
> not that I distrust you personally (hey, *everybody* lies to me 8-)

Aww Hans-Peter, would I ever lie to you ? :-)

> I committed the following as obvious (when you consider the new
> comment and the usage of the table) after build+test for cris-elf.

Thanks.

> -  { STRING_COMMA_LEN (".stabstr"),  3, SHT_STRTAB, 0 },
> +  /* See struct bfd_elf_special_section declaration for the semantics of
> +     this special case where .prefix_length != strlen (.prefix).  */
> +  { ".stabstr",			5,  3, SHT_STRTAB, 0 },
>    { NULL,                       0,  0, 0,          0 }

Well Doh.  Serves me right for trusting my build test and compare 
harness.  I guess I am going to have to go back and find out why this 
did not show up as a fix when I reverted my original patch.

Cheers
   Nick

Re: [PATCH]: Re: Regression with strn-stuff

[Nick Clifton]

Hi Pedro,

> making ("a string") "" uncompilable.

> I don't think there is any way in C to implement the
> REMOVE_PARENS macro below, is there?

No, I think that the parentheses will only be removed when the 
expression is evaluated.

Cheers
   Nick

Re: [PATCH]: Re: Regression with strn-stuff

[Andreas Schwab]

Pedro Alves <pedro_alves@portugalmail.pt> writes:

> I don't think there is any way in C to implement the
> REMOVE_PARENS macro below, is there?
>
> #define REMOVE_PARENS(S) (?)
> REMOVE_PARENS(("a string")) => "A STRING"

If you know that the argument always contains an extra pair of parens you
can write this:

#define REMOVE_PARENS(S) S
#define foo(STR) mumble (REMOVE_PARENS STR "")

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Make the compiler do the math 2.

[Nick Clifton]

Hi Andreas,

> You can't use STRING_COMMA_LEN as part of a function call because it may
> be a macro:
> 
> ../../bfd/elflink.c:10069:47: error: macro "memcpy" requires 3 arguments, but only 2 given

Hmm, can you tell me how I can reproduce this please ?  (I agree that 
STRING_COMMA_LEN will not work if used inside another macro, but I have 
not come across a situation where mempcy is a macro).

Cheers
   Nick

Re: [PATCH] Make the compiler do the math 2.

[Andreas Schwab]

Nick Clifton <nickc@redhat.com> writes:

> Hi Andreas,
>
>> You can't use STRING_COMMA_LEN as part of a function call because it may
>> be a macro:
>>
>> ../../bfd/elflink.c:10069:47: error: macro "memcpy" requires 3 arguments, but only 2 given
>
> Hmm, can you tell me how I can reproduce this please ?

#define memcpy(a,b,c) memcpy(a,b,c)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Make the compiler do the math 2.

[Hans-Peter Nilsson]

On Mon, 25 Sep 2006, Andreas Schwab wrote:
> ../../bfd/elflink.c:10069:47: error: macro "memcpy" requires 3 arguments, but only 2 given

For elflink.c 1.230 it's line 9707:
                memcpy (fn_name, STRING_COMMA_LEN (".text."));

perhaps most easily fixed by simply disabling use of any
memcpy-macro:
                (memcpy) (fn_name, STRING_COMMA_LEN (".text."));

brgds, H-P

Re: [PATCH] Make the compiler do the math 2.

[Andreas Schwab]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> On Mon, 25 Sep 2006, Andreas Schwab wrote:
>> ../../bfd/elflink.c:10069:47: error: macro "memcpy" requires 3 arguments, but only 2 given
>
> For elflink.c 1.230 it's line 9707:
>                 memcpy (fn_name, STRING_COMMA_LEN (".text."));
>
> perhaps most easily fixed by simply disabling use of any
> memcpy-macro:
>                 (memcpy) (fn_name, STRING_COMMA_LEN (".text."));

How about just using strcpy?  Also, the surrounding code has too many
occurrences of magic numbers that should be replaced.  Like this perhaps:

2006-09-25  Andreas Schwab  <schwab@suse.de>

bfd/
	* elflink.c (bfd_elf_gc_sections): Don't use STRING_COMMA_LEN as
	argument of memcpy.  Replace magic numbers.

binutils/
	* prdbg.c (tg_class_static_member): Don't use STRING_COMMA_LEN as
	argument of memcpy.

--- bfd/elflink.c.~1.230.~	2006-09-18 10:58:08.000000000 +0200
+++ bfd/elflink.c	2006-09-25 22:34:35.000000000 +0200
@@ -9699,13 +9699,16 @@ bfd_elf_gc_sections (bfd *abfd, struct b
 		unsigned long len;
 		char *fn_name;
 		asection *fn_text;
+		int o_name_prefix_len = strlen (".gcc_except_table.");
+		int fn_name_prefix_len = strlen (".text.");
 
-		len = strlen (o->name + 18) + 1;
-		fn_name = bfd_malloc (len + 6);
+		len = strlen (o->name + o_name_prefix_len) + 1;
+		fn_name = bfd_malloc (len + fn_name_prefix_len);
 		if (fn_name == NULL)
 		  return FALSE;
-		memcpy (fn_name, STRING_COMMA_LEN (".text."));
-		memcpy (fn_name + 6, o->name + 18, len);
+		strcpy (fn_name, ".text.");
+		memcpy (fn_name + fn_name_prefix_len,
+			o->name + o_name_prefix_len, len);
 		fn_text = bfd_get_section_by_name (sub, fn_name);
 		free (fn_name);
 		if (fn_text == NULL || !fn_text->gc_mark)
--- binutils/prdbg.c.~1.14.~	2006-09-18 10:58:08.000000000 +0200
+++ binutils/prdbg.c	2006-09-25 22:34:53.000000000 +0200
@@ -2157,7 +2157,7 @@ tg_class_static_member (void *p, const c
   if (! full_name)
     return FALSE;
   memcpy (full_name, info->stack->next->type, len_class);
-  memcpy (full_name + len_class, STRING_COMMA_LEN ("::"));
+  strcpy (full_name + len_class, "::");
   memcpy (full_name + len_class + 2, name, len_var + 1);
 
   if (! substitute_type (info, full_name))

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Make the compiler do the math 2.

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

Hans-Peter Nilsson escreveu:
> On Mon, 25 Sep 2006, Andreas Schwab wrote:
>   
>> ../../bfd/elflink.c:10069:47: error: macro "memcpy" requires 3 arguments, but only 2 given
>>     
>
> For elflink.c 1.230 it's line 9707:
>                 memcpy (fn_name, STRING_COMMA_LEN (".text."));
>
> perhaps most easily fixed by simply disabling use of any
> memcpy-macro:
>                 (memcpy) (fn_name, STRING_COMMA_LEN (".text."));
>
> brgds, H-P
>
>   
Or simply replace with:

-memcpy (fn_name, STRING_COMMA_LEN (".text."));

+CONST_STRNCPY (fn_name, ".text.")

Although all cases of CONST_STRNCPY would
be better replaced with something using memcpy,
since strncpy has to check if the null terminator arrives before the 
specified len, and memcpy,
obviously doesn't. That is, if the compiler doesn't optimize this away.
Also the return of CONST_STRNCPY is nowhere used, so this should be safe.

How about the attached patch?
I named the new macro LITCPY, since I think it serves more the purpose 
of the
macro.

Please (call me stubborn ;) , ) review, and commit.

Cheers,
Pedro Alves

---

bfd/ChangeLog

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

    * bfd-in.h (CONST_STRNCPY) : Delete.
    (LITCPY) : New.
    * bfd-in2.h : Regenerate.
    * elflink.c  (bfd_elf_gc_sections) : Use LITCPY. Don't manually 
calculate string lengths.
    * nlmcode.h (nlm_swap_auxiliary_headers_in) : Use LITCPY.

binutils/ChangeLog

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

    * nlmconv.c (main) : Use LITCPY.
    * prdbg.c (tg_class_static_member) : Likewise.


[-- Attachment #2: litcpy.diff --]
[-- Type: text/plain, Size: 7590 bytes --]

Index: bfd/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/bfd-in.h	16 Sep 2006 18:12:13 -0000	1.117
+++ bfd/bfd-in.h	25 Sep 2006 22:01:24 -0000
@@ -59,7 +59,7 @@ extern "C" {
    
    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)
+#define LITCPY(DEST,STR) memcpy ((DEST), (STR), sizeof (STR) - 1)
 
 
 /* The word size used by BFD on the host.  This may be 64 with a 32
Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.230
diff -u -p -r1.230 elflink.c
--- bfd/elflink.c	16 Sep 2006 18:12:14 -0000	1.230
+++ bfd/elflink.c	25 Sep 2006 22:01:50 -0000
@@ -9704,8 +9704,9 @@ bfd_elf_gc_sections (bfd *abfd, struct b
 		fn_name = bfd_malloc (len + 6);
 		if (fn_name == NULL)
 		  return FALSE;
-		memcpy (fn_name, STRING_COMMA_LEN (".text."));
-		memcpy (fn_name + 6, o->name + 18, len);
+		LITCPY (fn_name, ".text.");
+		memcpy (fn_name + sizeof (".text.") - 1, o->name 
+				+ sizeof (".gcc_except_table.") - 1, len);
 		fn_text = bfd_get_section_by_name (sub, fn_name);
 		free (fn_name);
 		if (fn_text == NULL || !fn_text->gc_mark)
Index: bfd/nlmcode.h
===================================================================
RCS file: /cvs/src/src/bfd/nlmcode.h,v
retrieving revision 1.19
diff -u -p -r1.19 nlmcode.h
--- bfd/nlmcode.h	16 Sep 2006 18:12:14 -0000	1.19
+++ bfd/nlmcode.h	25 Sep 2006 22:01:55 -0000
@@ -364,7 +364,7 @@ nlm_swap_auxiliary_headers_in (bfd *abfd
 	      if (bfd_seek (abfd, pos, SEEK_SET) != 0)
 		return FALSE;
 
-	      memcpy (nlm_cygnus_ext_header (abfd), STRING_COMMA_LEN ("CyGnUsEx"));
+	      LITCPY (nlm_cygnus_ext_header (abfd), "CyGnUsEx");
 	      nlm_cygnus_ext_header (abfd)->offset = dataOffset;
 	      nlm_cygnus_ext_header (abfd)->length = dataLength;
 
@@ -645,7 +645,7 @@ nlm_swap_auxiliary_headers_out (bfd *abf
     {
       Nlm_External_Version_Header thdr;
 
-      memcpy (thdr.stamp, STRING_COMMA_LEN ("VeRsIoN#"));
+      LITCPY (thdr.stamp, "VeRsIoN#");
       put_word (abfd, (bfd_vma) nlm_version_header (abfd)->majorVersion,
 		(bfd_byte *) thdr.majorVersion);
       put_word (abfd, (bfd_vma) nlm_version_header (abfd)->minorVersion,
@@ -672,7 +672,7 @@ nlm_swap_auxiliary_headers_out (bfd *abf
     {
       Nlm_External_Copyright_Header thdr;
 
-      memcpy (thdr.stamp, STRING_COMMA_LEN ("CoPyRiGhT="));
+      LITCPY (thdr.stamp, "CoPyRiGhT=");
       amt = sizeof (thdr.stamp);
       if (bfd_bwrite ((void *) thdr.stamp, amt, abfd) != amt)
 	return FALSE;
@@ -694,7 +694,7 @@ nlm_swap_auxiliary_headers_out (bfd *abf
     {
       Nlm_External_Extended_Header thdr;
 
-      memcpy (thdr.stamp, STRING_COMMA_LEN ("MeSsAgEs"));
+      LITCPY (thdr.stamp, "MeSsAgEs");
       put_word (abfd,
 		(bfd_vma) nlm_extended_header (abfd)->languageID,
 		(bfd_byte *) thdr.languageID);
@@ -797,7 +797,7 @@ nlm_swap_auxiliary_headers_out (bfd *abf
 
       ds = find_nonzero (nlm_custom_header (abfd)->dataStamp,
 			 sizeof (nlm_custom_header (abfd)->dataStamp));
-      memcpy (thdr.stamp, STRING_COMMA_LEN ("CuStHeAd"));
+      LITCPY (thdr.stamp, "CuStHeAd");
       hdrLength = (2 * NLM_TARGET_LONG_SIZE + (ds ? 8 : 0)
 		   + nlm_custom_header (abfd)->hdrLength);
       put_word (abfd, hdrLength, thdr.length);
@@ -831,14 +831,14 @@ nlm_swap_auxiliary_headers_out (bfd *abf
     {
       Nlm_External_Custom_Header thdr;
 
-      memcpy (thdr.stamp, STRING_COMMA_LEN ("CuStHeAd"));
+      LITCPY (thdr.stamp, "CuStHeAd");
       put_word (abfd, (bfd_vma) 2 * NLM_TARGET_LONG_SIZE + 8,
 		(bfd_byte *) thdr.length);
       put_word (abfd, (bfd_vma) nlm_cygnus_ext_header (abfd)->offset,
 		(bfd_byte *) thdr.dataOffset);
       put_word (abfd, (bfd_vma) nlm_cygnus_ext_header (abfd)->length,
 		(bfd_byte *) thdr.dataLength);
-      memcpy (thdr.dataStamp, STRING_COMMA_LEN ("CyGnUsEx"));
+      LITCPY (thdr.dataStamp, "CyGnUsEx");
       amt = sizeof (thdr);
       if (bfd_bwrite ((void *) &thdr, amt, abfd) != amt)
 	return FALSE;
Index: binutils/nlmconv.c
===================================================================
RCS file: /cvs/src/src/binutils/nlmconv.c,v
retrieving revision 1.28
diff -u -p -r1.28 nlmconv.c
--- binutils/nlmconv.c	16 Sep 2006 18:12:17 -0000	1.28
+++ binutils/nlmconv.c	25 Sep 2006 22:01:59 -0000
@@ -737,7 +737,7 @@ main (int argc, char **argv)
 	      || ! bfd_set_section_flags (outbfd, help_section,
 					  SEC_HAS_CONTENTS))
 	    bfd_fatal (_("help section"));
-	  CONST_STRNCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
+	  LITCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
 	}
     }
   if (message_file != NULL)
@@ -759,7 +759,7 @@ main (int argc, char **argv)
 	      || ! bfd_set_section_flags (outbfd, message_section,
 					  SEC_HAS_CONTENTS))
 	    bfd_fatal (_("message section"));
-	  CONST_STRNCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
+	  LITCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
 	}
     }
   if (modules != NULL)
@@ -795,7 +795,7 @@ main (int argc, char **argv)
 	      || ! bfd_set_section_flags (outbfd, rpc_section,
 					  SEC_HAS_CONTENTS))
 	    bfd_fatal (_("rpc section"));
-	  CONST_STRNCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
+	  LITCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
 	}
     }
   if (sharelib_file != NULL)
@@ -852,7 +852,7 @@ main (int argc, char **argv)
 		  || ! bfd_set_section_flags (outbfd, shared_section,
 					      SEC_HAS_CONTENTS))
 		bfd_fatal (_("shared section"));
-	      CONST_STRNCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
+	      LITCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
 	    }
 	}
     }
@@ -863,9 +863,9 @@ main (int argc, char **argv)
 
   /* At least for now, always create an extended header, because that
      is what NLMLINK does.  */
-  CONST_STRNCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
+  LITCPY (nlm_extended_header (outbfd)->stamp, "MeSsAgEs");
 
-  CONST_STRNCPY (nlm_cygnus_ext_header (outbfd)->stamp, "CyGnUsEx");
+  LITCPY (nlm_cygnus_ext_header (outbfd)->stamp, "CyGnUsEx");
 
   /* If the date was not given, force it in.  */
   if (nlm_version_header (outbfd)->month == 0
@@ -880,7 +880,7 @@ main (int argc, char **argv)
       nlm_version_header (outbfd)->month = ptm->tm_mon + 1;
       nlm_version_header (outbfd)->day = ptm->tm_mday;
       nlm_version_header (outbfd)->year = ptm->tm_year + 1900;
-      CONST_STRNCPY (version_hdr->stamp, "VeRsIoN#");
+      LITCPY (version_hdr->stamp, "VeRsIoN#");
     }
 
 #ifdef NLMCONV_POWERPC
Index: binutils/prdbg.c
===================================================================
RCS file: /cvs/src/src/binutils/prdbg.c,v
retrieving revision 1.14
diff -u -p -r1.14 prdbg.c
--- binutils/prdbg.c	16 Sep 2006 18:12:17 -0000	1.14
+++ binutils/prdbg.c	25 Sep 2006 22:02:04 -0000
@@ -2157,8 +2157,8 @@ tg_class_static_member (void *p, const c
   if (! full_name)
     return FALSE;
   memcpy (full_name, info->stack->next->type, len_class);
-  memcpy (full_name + len_class, STRING_COMMA_LEN ("::"));
-  memcpy (full_name + len_class + 2, name, len_var + 1);
+  LITCPY (full_name + len_class, "::");
+  memcpy (full_name + len_class + sizeof ("::") - 1, name, len_var + 1);
 
   if (! substitute_type (info, full_name))
     return FALSE;

Re: [PATCH] Make the compiler do the math 2.

[Nick Clifton]

Hi Andreas,

> How about just using strcpy?  

strcpy also copies the NUL byte at the end of the string, which may not 
be desired in all circumstances, (although in this particular case it is 
OK).

> Also, the surrounding code has too many
> occurrences of magic numbers that should be replaced. 

A very good idea.

> Like this perhaps:

> --- bfd/elflink.c.~1.230.~	2006-09-18 10:58:08.000000000 +0200
> +++ bfd/elflink.c	2006-09-25 22:34:35.000000000 +0200
> @@ -9699,13 +9699,16 @@ bfd_elf_gc_sections (bfd *abfd, struct b
>  		unsigned long len;
>  		char *fn_name;
>  		asection *fn_text;
> +		int o_name_prefix_len = strlen (".gcc_except_table.");
> +		int fn_name_prefix_len = strlen (".text.");
>  
> -		len = strlen (o->name + 18) + 1;
> -		fn_name = bfd_malloc (len + 6);
> +		len = strlen (o->name + o_name_prefix_len) + 1;
> +		fn_name = bfd_malloc (len + fn_name_prefix_len);
>  		if (fn_name == NULL)
>  		  return FALSE;
> -		memcpy (fn_name, STRING_COMMA_LEN (".text."));
> -		memcpy (fn_name + 6, o->name + 18, len);
> +		strcpy (fn_name, ".text.");
> +		memcpy (fn_name + fn_name_prefix_len,
> +			o->name + o_name_prefix_len, len);
>  		fn_text = bfd_get_section_by_name (sub, fn_name);
>  		free (fn_name);
>  		if (fn_text == NULL || !fn_text->gc_mark)

I like this, although for my money I think that we would be better off 
replacing the multiple occurrences of the same constant string with a 
#define, just to make it clear that we are intentionally dealing with 
the same piece of text.  eg:

   #define DOT_TEXT_DOT ".text."
   ...
   int fn_name_prefix_len = sizeof (DOT_TEXT_DOT) - 1;
   ...
   strcpy (fn_name, DOT_TEXT_DOT);

Cheers
   Nick

Re: [PATCH] Make the compiler do the math 2.

[Nick Clifton]

Hi Andreas, Hans-Peter, Pedro,

> --- bfd/elflink.c.~1.230.~	2006-09-18 10:58:08.000000000 +0200
> +++ bfd/elflink.c	2006-09-25 22:34:35.000000000 +0200
> @@ -9699,13 +9699,16 @@ bfd_elf_gc_sections (bfd *abfd, struct b
>  		unsigned long len;
>  		char *fn_name;
>  		asection *fn_text;
> +		int o_name_prefix_len = strlen (".gcc_except_table.");
> +		int fn_name_prefix_len = strlen (".text.");
>  
> -		len = strlen (o->name + 18) + 1;
> -		fn_name = bfd_malloc (len + 6);
> +		len = strlen (o->name + o_name_prefix_len) + 1;
> +		fn_name = bfd_malloc (len + fn_name_prefix_len);
>  		if (fn_name == NULL)
>  		  return FALSE;
> -		memcpy (fn_name, STRING_COMMA_LEN (".text."));
> -		memcpy (fn_name + 6, o->name + 18, len);
> +		strcpy (fn_name, ".text.");
> +		memcpy (fn_name + fn_name_prefix_len,
> +			o->name + o_name_prefix_len, len);
>  		fn_text = bfd_get_section_by_name (sub, fn_name);
>  		free (fn_name);
>  		if (fn_text == NULL || !fn_text->gc_mark)

You know, I was looking at this code, and tweaking it with Pedro's 
version of the patch when it occurred to me that using memcpy or strcpy 
here is not the best thing to do.  It obscures what is going on, and 
whilst it may be more efficient, it makes the code harder to read. 
Personally I would prefer to use the slower sprintf() function as I 
think that makes things clearer.  Like this:

   if (CONST_STRNEQ (o->name, GCC_EXCEPT_TABLE))
     {
       char *fn_name;
       char *sec_name;
       asection *fn_text;
       unsigned o_name_prefix_len  = sizeof (GCC_EXCEPT_TABLE) - 1;
       unsigned fn_name_prefix_len = sizeof (DOT_TEXT_DOT) - 1;

       sec_name = o->name + o_name_prefix_len;
       fn_name = bfd_malloc (strlen (sec_name) + fn_name_prefix_len + 1);
       if (fn_name == NULL)
         return FALSE;
       sprintf (fn_name, DOT_TEXT_DOT "%s", sec_name);
       fn_text = bfd_get_section_by_name (sub, fn_name);
       free (fn_name);
       if (fn_text == NULL || !fn_text->gc_mark)
         continue;
     }

   With suitable #define's for DOT_TEXT_DOT and GCC_EXCEPT_TABLE of 
course.  What do you think ?

   Actually I think that it might be clearer if we used two %s operators 
inside the sprintf like this:

       sprintf (fn_name, "%s%s", DOT_TEXT_DOT, sec_name);

Cheers
   Nick

Re: [PATCH] Make the compiler do the math 2.

[Andreas Schwab]

Nick Clifton <nickc@redhat.com> writes:

> Like this:
>
>   if (CONST_STRNEQ (o->name, GCC_EXCEPT_TABLE))
>     {
>       char *fn_name;
>       char *sec_name;
>       asection *fn_text;
>       unsigned o_name_prefix_len  = sizeof (GCC_EXCEPT_TABLE) - 1;
>       unsigned fn_name_prefix_len = sizeof (DOT_TEXT_DOT) - 1;

I'd make that use strlen anyway.  It more clearly states the intent and
the compiler can optimize it out nevertheless.

>   Actually I think that it might be clearer if we used two %s operators
> inside the sprintf like this:
>
>       sprintf (fn_name, "%s%s", DOT_TEXT_DOT, sec_name);

I agree.  Again, the compiler is allowed to optimize sprintf out of
existence.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Make the compiler do the math 2.

[Pedro Alves]

Nick Clifton wrote:
> Hi Andreas, Hans-Peter, Pedro,
> 
>> --- bfd/elflink.c.~1.230.~    2006-09-18 10:58:08.000000000 +0200
>> +++ bfd/elflink.c    2006-09-25 22:34:35.000000000 +0200
>> @@ -9699,13 +9699,16 @@ bfd_elf_gc_sections (bfd *abfd, struct b
>>          unsigned long len;
>>          char *fn_name;
>>          asection *fn_text;
>> +        int o_name_prefix_len = strlen (".gcc_except_table.");
>> +        int fn_name_prefix_len = strlen (".text.");
>>  
>> -        len = strlen (o->name + 18) + 1;
>> -        fn_name = bfd_malloc (len + 6);
>> +        len = strlen (o->name + o_name_prefix_len) + 1;
>> +        fn_name = bfd_malloc (len + fn_name_prefix_len);
>>          if (fn_name == NULL)
>>            return FALSE;
>> -        memcpy (fn_name, STRING_COMMA_LEN (".text."));
>> -        memcpy (fn_name + 6, o->name + 18, len);
>> +        strcpy (fn_name, ".text.");
>> +        memcpy (fn_name + fn_name_prefix_len,
>> +            o->name + o_name_prefix_len, len);
>>          fn_text = bfd_get_section_by_name (sub, fn_name);
>>          free (fn_name);
>>          if (fn_text == NULL || !fn_text->gc_mark)
> 
> 
> You know, I was looking at this code, and tweaking it with Pedro's 
> version of the patch when it occurred to me that using memcpy or strcpy 
> here is not the best thing to do.  It obscures what is going on, and 
> whilst it may be more efficient, it makes the code harder to read. 
> Personally I would prefer to use the slower sprintf() function as I 
> think that makes things clearer.  Like this:
> 
>   if (CONST_STRNEQ (o->name, GCC_EXCEPT_TABLE))
>     {
>       char *fn_name;
>       char *sec_name;
>       asection *fn_text;
>       unsigned o_name_prefix_len  = sizeof (GCC_EXCEPT_TABLE) - 1;
>       unsigned fn_name_prefix_len = sizeof (DOT_TEXT_DOT) - 1;
> 
>       sec_name = o->name + o_name_prefix_len;
>       fn_name = bfd_malloc (strlen (sec_name) + fn_name_prefix_len + 1);
>       if (fn_name == NULL)
>         return FALSE;
>       sprintf (fn_name, DOT_TEXT_DOT "%s", sec_name);
>       fn_text = bfd_get_section_by_name (sub, fn_name);
>       free (fn_name);
>       if (fn_text == NULL || !fn_text->gc_mark)
>         continue;
>     }
> 
>   With suitable #define's for DOT_TEXT_DOT and GCC_EXCEPT_TABLE of 
> course.  What do you think ?
> 
>   Actually I think that it might be clearer if we used two %s operators 
> inside the sprintf like this:
> 
>       sprintf (fn_name, "%s%s", DOT_TEXT_DOT, sec_name);
> 

Agreed.

Cheers,
Pedro Alves

> Cheers
>   Nick

Re: [PATCH] Make the compiler do the math 2.

[Nick Clifton]

Hi Pedro,

> bfd/ChangeLog
> 
> 2006-09-25 Pedro Alves <pedro_alves@portugalmail.pt>
> 
>    * bfd-in.h (CONST_STRNCPY) : Delete.
>    (LITCPY) : New.
>    * bfd-in2.h : Regenerate.
>    * elflink.c  (bfd_elf_gc_sections) : Use LITCPY. Don't manually 
> calculate string lengths.
>    * nlmcode.h (nlm_swap_auxiliary_headers_in) : Use LITCPY.
> 
> binutils/ChangeLog
> 
> 2006-09-25 Pedro Alves <pedro_alves@portugalmail.pt>
> 
>    * nlmconv.c (main) : Use LITCPY.
>    * prdbg.c (tg_class_static_member) : Likewise.

(Much checking later...) Approved and applied.  I renamed the macro to 
LITMEMCPY so that we could also have a LITSTRCPY for when we really do 
want to copy the NUL at the end of the string.

Cheers
   Nick