* [PATCH] Make the compiler do the math 2.
@ 2006-09-11 21:03 Pedro Alves
2006-09-12 11:27 ` Pedro Alves
0 siblings, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2006-09-11 21:03 UTC (permalink / raw)
To: binutils
[-- 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] == '.')
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-11 21:03 [PATCH] Make the compiler do the math 2 Pedro Alves
@ 2006-09-12 11:27 ` Pedro Alves
2006-09-16 18:16 ` Nick Clifton
0 siblings, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2006-09-12 11:27 UTC (permalink / raw)
To: binutils
[-- 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] == '.')
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-12 11:27 ` Pedro Alves
@ 2006-09-16 18:16 ` Nick Clifton
2006-09-17 3:39 ` Regression with strn-stuff (was: Re: [PATCH] Make the compiler do the math 2.) Hans-Peter Nilsson
2006-09-25 14:01 ` [PATCH] Make the compiler do the math 2 Andreas Schwab
0 siblings, 2 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-16 18:16 UTC (permalink / raw)
To: Pedro Alves; +Cc: binutils
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Regression with strn-stuff (was: Re: [PATCH] Make the compiler do the math 2.)
2006-09-16 18:16 ` Nick Clifton
@ 2006-09-17 3:39 ` Hans-Peter Nilsson
2006-09-17 14:41 ` Regression with strn-stuff Nick Clifton
2006-09-25 14:01 ` [PATCH] Make the compiler do the math 2 Andreas Schwab
1 sibling, 1 reply; 24+ messages in thread
From: Hans-Peter Nilsson @ 2006-09-17 3:39 UTC (permalink / raw)
To: nickc; +Cc: binutils
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Regression with strn-stuff
2006-09-17 3:39 ` Regression with strn-stuff (was: Re: [PATCH] Make the compiler do the math 2.) Hans-Peter Nilsson
@ 2006-09-17 14:41 ` Nick Clifton
2006-09-17 18:58 ` Pedro Alves
2006-09-17 19:08 ` Hans-Peter Nilsson
0 siblings, 2 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-17 14:41 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Regression with strn-stuff
2006-09-17 14:41 ` Regression with strn-stuff Nick Clifton
@ 2006-09-17 18:58 ` Pedro Alves
2006-09-17 19:17 ` Hans-Peter Nilsson
2006-09-17 19:08 ` Hans-Peter Nilsson
1 sibling, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2006-09-17 18:58 UTC (permalink / raw)
To: Nick Clifton; +Cc: Hans-Peter Nilsson, binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Regression with strn-stuff
2006-09-17 14:41 ` Regression with strn-stuff Nick Clifton
2006-09-17 18:58 ` Pedro Alves
@ 2006-09-17 19:08 ` Hans-Peter Nilsson
2006-09-25 16:09 ` Nick Clifton
1 sibling, 1 reply; 24+ messages in thread
From: Hans-Peter Nilsson @ 2006-09-17 19:08 UTC (permalink / raw)
To: nickc; +Cc: binutils
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Regression with strn-stuff
2006-09-17 18:58 ` Pedro Alves
@ 2006-09-17 19:17 ` Hans-Peter Nilsson
2006-09-17 22:20 ` [PATCH]: " Pedro Alves
0 siblings, 1 reply; 24+ messages in thread
From: Hans-Peter Nilsson @ 2006-09-17 19:17 UTC (permalink / raw)
To: pedro_alves; +Cc: nickc, hans-peter.nilsson, binutils
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH]: Re: Regression with strn-stuff
2006-09-17 19:17 ` Hans-Peter Nilsson
@ 2006-09-17 22:20 ` Pedro Alves
2006-09-19 10:37 ` Pedro Alves
0 siblings, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2006-09-17 22:20 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: nickc, binutils
[-- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH]: Re: Regression with strn-stuff
2006-09-17 22:20 ` [PATCH]: " Pedro Alves
@ 2006-09-19 10:37 ` Pedro Alves
2006-09-25 16:14 ` Nick Clifton
2006-09-25 16:19 ` Andreas Schwab
0 siblings, 2 replies; 24+ messages in thread
From: Pedro Alves @ 2006-09-19 10:37 UTC (permalink / raw)
To: Pedro Alves; +Cc: Hans-Peter Nilsson, nickc, binutils
>>> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-16 18:16 ` Nick Clifton
2006-09-17 3:39 ` Regression with strn-stuff (was: Re: [PATCH] Make the compiler do the math 2.) Hans-Peter Nilsson
@ 2006-09-25 14:01 ` Andreas Schwab
2006-09-25 17:14 ` Nick Clifton
2006-09-25 20:38 ` Hans-Peter Nilsson
1 sibling, 2 replies; 24+ messages in thread
From: Andreas Schwab @ 2006-09-25 14:01 UTC (permalink / raw)
To: Nick Clifton; +Cc: Pedro Alves, binutils
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."
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Regression with strn-stuff
2006-09-17 19:08 ` Hans-Peter Nilsson
@ 2006-09-25 16:09 ` Nick Clifton
0 siblings, 0 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-25 16:09 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH]: Re: Regression with strn-stuff
2006-09-19 10:37 ` Pedro Alves
@ 2006-09-25 16:14 ` Nick Clifton
2006-09-25 16:19 ` Andreas Schwab
1 sibling, 0 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-25 16:14 UTC (permalink / raw)
To: Pedro Alves; +Cc: Hans-Peter Nilsson, binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH]: Re: Regression with strn-stuff
2006-09-19 10:37 ` Pedro Alves
2006-09-25 16:14 ` Nick Clifton
@ 2006-09-25 16:19 ` Andreas Schwab
1 sibling, 0 replies; 24+ messages in thread
From: Andreas Schwab @ 2006-09-25 16:19 UTC (permalink / raw)
To: Pedro Alves; +Cc: Hans-Peter Nilsson, nickc, binutils
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."
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 14:01 ` [PATCH] Make the compiler do the math 2 Andreas Schwab
@ 2006-09-25 17:14 ` Nick Clifton
2006-09-25 19:37 ` Andreas Schwab
2006-09-25 20:38 ` Hans-Peter Nilsson
1 sibling, 1 reply; 24+ messages in thread
From: Nick Clifton @ 2006-09-25 17:14 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Pedro Alves, binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 17:14 ` Nick Clifton
@ 2006-09-25 19:37 ` Andreas Schwab
0 siblings, 0 replies; 24+ messages in thread
From: Andreas Schwab @ 2006-09-25 19:37 UTC (permalink / raw)
To: Nick Clifton; +Cc: Pedro Alves, binutils
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."
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 14:01 ` [PATCH] Make the compiler do the math 2 Andreas Schwab
2006-09-25 17:14 ` Nick Clifton
@ 2006-09-25 20:38 ` Hans-Peter Nilsson
2006-09-25 22:23 ` Andreas Schwab
2006-09-25 23:53 ` Pedro Alves
1 sibling, 2 replies; 24+ messages in thread
From: Hans-Peter Nilsson @ 2006-09-25 20:38 UTC (permalink / raw)
To: Andreas Schwab; +Cc: binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 20:38 ` Hans-Peter Nilsson
@ 2006-09-25 22:23 ` Andreas Schwab
2006-09-26 8:49 ` Nick Clifton
2006-09-26 10:17 ` Nick Clifton
2006-09-25 23:53 ` Pedro Alves
1 sibling, 2 replies; 24+ messages in thread
From: Andreas Schwab @ 2006-09-25 22:23 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: binutils
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."
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 20:38 ` Hans-Peter Nilsson
2006-09-25 22:23 ` Andreas Schwab
@ 2006-09-25 23:53 ` Pedro Alves
2006-09-28 13:25 ` Nick Clifton
1 sibling, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2006-09-25 23:53 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: Andreas Schwab, binutils
[-- 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;
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 22:23 ` Andreas Schwab
@ 2006-09-26 8:49 ` Nick Clifton
2006-09-26 10:17 ` Nick Clifton
1 sibling, 0 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-26 8:49 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Hans-Peter Nilsson, binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 22:23 ` Andreas Schwab
2006-09-26 8:49 ` Nick Clifton
@ 2006-09-26 10:17 ` Nick Clifton
2006-09-26 10:30 ` Andreas Schwab
2006-09-26 12:38 ` Pedro Alves
1 sibling, 2 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-26 10:17 UTC (permalink / raw)
To: Andreas Schwab, Hans-Peter Nilsson, Pedro Alves; +Cc: binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-26 10:17 ` Nick Clifton
@ 2006-09-26 10:30 ` Andreas Schwab
2006-09-26 12:38 ` Pedro Alves
1 sibling, 0 replies; 24+ messages in thread
From: Andreas Schwab @ 2006-09-26 10:30 UTC (permalink / raw)
To: Nick Clifton; +Cc: Hans-Peter Nilsson, Pedro Alves, binutils
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."
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-26 10:17 ` Nick Clifton
2006-09-26 10:30 ` Andreas Schwab
@ 2006-09-26 12:38 ` Pedro Alves
1 sibling, 0 replies; 24+ messages in thread
From: Pedro Alves @ 2006-09-26 12:38 UTC (permalink / raw)
To: Nick Clifton; +Cc: Andreas Schwab, Hans-Peter Nilsson, binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Make the compiler do the math 2.
2006-09-25 23:53 ` Pedro Alves
@ 2006-09-28 13:25 ` Nick Clifton
0 siblings, 0 replies; 24+ messages in thread
From: Nick Clifton @ 2006-09-28 13:25 UTC (permalink / raw)
To: Pedro Alves; +Cc: Hans-Peter Nilsson, Andreas Schwab, binutils
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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2006-09-28 13:00 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-09-11 21:03 [PATCH] Make the compiler do the math 2 Pedro Alves
2006-09-12 11:27 ` Pedro Alves
2006-09-16 18:16 ` Nick Clifton
2006-09-17 3:39 ` Regression with strn-stuff (was: Re: [PATCH] Make the compiler do the math 2.) Hans-Peter Nilsson
2006-09-17 14:41 ` Regression with strn-stuff Nick Clifton
2006-09-17 18:58 ` Pedro Alves
2006-09-17 19:17 ` Hans-Peter Nilsson
2006-09-17 22:20 ` [PATCH]: " Pedro Alves
2006-09-19 10:37 ` Pedro Alves
2006-09-25 16:14 ` Nick Clifton
2006-09-25 16:19 ` Andreas Schwab
2006-09-17 19:08 ` Hans-Peter Nilsson
2006-09-25 16:09 ` Nick Clifton
2006-09-25 14:01 ` [PATCH] Make the compiler do the math 2 Andreas Schwab
2006-09-25 17:14 ` Nick Clifton
2006-09-25 19:37 ` Andreas Schwab
2006-09-25 20:38 ` Hans-Peter Nilsson
2006-09-25 22:23 ` Andreas Schwab
2006-09-26 8:49 ` Nick Clifton
2006-09-26 10:17 ` Nick Clifton
2006-09-26 10:30 ` Andreas Schwab
2006-09-26 12:38 ` Pedro Alves
2006-09-25 23:53 ` Pedro Alves
2006-09-28 13:25 ` Nick Clifton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).