* [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 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]: 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: 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 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] 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: [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 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 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 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).