Do not copy to same address
Subsystems (2)
PATCH 1/1 Do not copy to same address
In some cases (e.g. bootm with a elf payload) there is a in place copy of data to the same address. Catching this saves some ms while booting.
--- lib/string.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
lib/string.c +9 -0
@@ -445,6 +445,9 @@ char * bcopy(const char * src, char * dest, int count) {char *tmp = dest; +if (src == dest) +return dest; +while (count--) *tmp++ = *src++;
@@ -467,6 +470,9 @@ void * memcpy(void *dest, const void *src, size_t count)unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;char *d8, *s8; +if (src == dest) +return dest; +/* while all data is aligned (common case), copy a word at a time */ if ( (((ulong )dest | (ulong )src) & (sizeof (*dl) -1 )) ==0 ) {while (count >=sizeof (*dl)) {
@@ -497,6 +503,9 @@ void * memmove(void * dest,const void *src,size_t count) {char *tmp, *s; +if (src == dest) +return dest; +if (dest <= src) { tmp = (char *) dest; s = (char *) src;
Signature · 1 line
1.7.0.4
PATCH 1/1 Do not copy to same address
this summary is kind of weak. please prefix it with something like "string:" or "memcpy/memmove:". keep in mind that the summary needs to quickly pick out what the changeset is doing from every other changeset in the tree based only on that. or at least give a pretty good idea.
side note, i wonder why we even bother with bcopy at all. i dont see any users of it in the tree ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110412/ea5eb6d6/attachment.pgp
PATCH 1/1 Do not copy to same address
Am 12.04.2011 09:05, schrieb Mike Frysinger:
Quoted 4 lines
this summary is kind of weak. please prefix it with something like "string:" or "memcpy/memmove:". keep in mind that the summary needs to quickly pick out what the changeset is doing from every other changeset in the tree based only on that. or at least give a pretty good idea.
OK. I will wait for additional comments and post a V2 then.
Quoted 2 lines
side note, i wonder why we even bother with bcopy at all. i dont see any users of it in the tree ...
I see the same. But removal of bcopy should be a separate patch.
Regards,
Matthias
PATCH 1/1 Do not copy to same address
In reply to Matthias WeisserHi Matthias,
Le 12/04/2011 08:58, Matthias Weisser a ?crit :
Quoted 7 lines
In some cases (e.g. bootm with a elf payload) there is a in place copy of data to the same address. Catching this saves some ms while booting.
Signed-off-by Matthias Weisser<[email protected]>--- lib/string.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
I believe that is a V2 patch, right? Please tag it as V2 in the subject line, and add patch history below the commit message delimiter ('---' ).
Amicalement,
Signature · 1 line
Albert.
PATCH 1/1 Do not copy to same address
Am 12.04.2011 09:06, schrieb Albert ARIBAUD:
Quoted 6 lines
Hi Matthias,
Le 12/04/2011 08:58, Matthias Weisser a ?crit :
Quoted 3 lines
In some cases (e.g. bootm with a elf payload) there is a in place copy of data to the same address. Catching this saves some ms while booting.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Matthias Weisser<[email protected]>--- lib/string.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)I believe that is a V2 patch, right? Please tag it as V2 in the subject line, and add patch history below the commit message delimiter ('---' ).
No, it is a replacement for http://patchwork.ozlabs.org/patch/79447/
which picks up a suggestion from Wolfgang.
Regards,
Matthias
PATCH 1/1 Do not copy to same address
Hi Matthias,
Le 12/04/2011 09:13, Matthias Wei?er a ?crit :
Quoted 13 lines
Am 12.04.2011 09:06, schrieb Albert ARIBAUD:
Quoted 12 lines
Hi Matthias,
Le 12/04/2011 08:58, Matthias Weisser a ?crit :
Quoted 8 lines
In some cases (e.g. bootm with a elf payload) there is a in place copy of
data to the same address. Catching this saves some ms while booting.Signed-off-by Matthias Weisser<[email protected]>---
lib/string.c| 9 +++++++++1 files changed, 9 insertions(+), 0 deletions(-)
Quoted 5 lines
Quoted 2 lines
I believe that is a V2 patch, right? Please tag it as V2 in the subject line, and add patch history below the commit message delimiter ('---' ).
No, it is a replacement for http://patchwork.ozlabs.org/patch/79447/
which picks up a suggestion from Wolfgang.
So it is a V3, not V2, but still you must tag it so that readers who see the previous patch can relate it to the 'replacement' -- yes, even if the files touched by V2 are different.
Quoted 2 lines
Regards,
Matthias
Amicalement,
Signature · 1 line
Albert.
PATCH 1/1 Do not copy to same address
Am 12.04.2011 09:27, schrieb Albert ARIBAUD:
Quoted 19 lines
Hi Matthias,
Le 12/04/2011 09:13, Matthias Wei?er a ?crit :
Quoted 16 lines
Am 12.04.2011 09:06, schrieb Albert ARIBAUD:
Quoted 14 lines
Hi Matthias,
Le 12/04/2011 08:58, Matthias Weisser a ?crit :
Quoted 8 lines
In some cases (e.g. bootm with a elf payload) there is a in place copy of
data to the same address. Catching this saves some ms while booting.Signed-off-by Matthias Weisser<[email protected]>---
lib/string.c| 9 +++++++++1 files changed, 9 insertions(+), 0 deletions(-)I believe that is a V2 patch, right? Please tag it as V2 in the subject line, and add patch history below the commit message delimiter ('---' ).
Quoted 6 lines
Quoted 2 lines
No, it is a replacement for http://patchwork.ozlabs.org/patch/79447/
which picks up a suggestion from Wolfgang.So it is a V3, not V2, but still you must tag it so that readers who see the previous patch can relate it to the 'replacement' -- yes, even if the files touched by V2 are different.
Well, as the patch is only slightly related to my original one I thought it is better to start a new patch as I had to change the subject also. The only relation between them would be the reference in the mail header. Maybe Wolfgang can bring some light into this situation.
What would be the right way to post this patch? And what would be the right way to post a patch doing exactly the same to the ARM optimized version of memcpy?
Sorry for all that administrative questions.
Regards,
Matthias
PATCH 1/1 Do not copy to same address
Hi Matthias,
Le 12/04/2011 09:49, Matthias Wei?er a ?crit :
Quoted 6 lines
Well, as the patch is only slightly related to my original one I thought it is better to start a new patch as I had to change the subject also. The only relation between them would be the reference in the mail header. Maybe Wolfgang can bring some light into this situation.
What would be the right way to post this patch?
Never mind with the previous patch, then; but please keep history of this one (maybe including a patchwork URL pointing to the previous patch with the other name) and tag its V2 in the subject line.
Quoted 3 lines
And what would be the
right way to post a patch doing exactly the same to the ARM optimized version of memcpy?
Simply submit a separate patch.
Quoted 2 lines
Regards,
Matthias
Amicalement,
Signature · 1 line
Albert.
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
In reply to Matthias WeisserIn some cases (e.g. bootm with a elf payload which is already at the right position) there is a in place copy of data to the same address. Catching this saves some ms while booting.
---
Changes since V1:
- Made subject more informative
- Removed the optimization from bcopy as bcopy is not used anywhere
lib/string.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
lib/string.c +6 -0
@@ -467,6 +467,9 @@ void * memcpy(void *dest, const void *src, size_t count)unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;char *d8, *s8; +if (src == dest) +return dest; +/* while all data is aligned (common case), copy a word at a time */ if ( (((ulong )dest | (ulong )src) & (sizeof (*dl) -1 )) ==0 ) {while (count >=sizeof (*dl)) {
@@ -497,6 +500,9 @@ void * memmove(void * dest,const void *src,size_t count) {char *tmp, *s; +if (src == dest) +return dest; +if (dest <= src) { tmp = (char *) dest; s = (char *) src;
Signature · 1 line
1.7.0.4
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Hello,
Am 23.05.2011 11:03, schrieb Matthias Weisser:
Quoted 18 lines
In some cases (e.g. bootm with a elf payload which is already at the right position) there is a in place copy of data to the same address. Catching this saves some ms while booting.
Signed-off-by Matthias Weisser<[email protected]>---
Changes since V1:
- Made subject more informative
- Removed the optimization from bcopy as bcopy is not used anywherelib/string.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)lib/string.c +0
-0
lib/string.c void * memcpy(void *dest, const void *src, size_t count) · line 472
Quoted 6 lines
lib/string.c +3
-0@@ -467,6 +467,9 @@ void * memcpy(void *dest, const void *src, size_t count)unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;char *d8, *s8; +if (src == dest) +return dest; +
here is the same, as in the patch I've commented before. There exist no reason to add a check for identity to memcpy() because memcpy doesn't support overlapping regions (and identity is just a special case of overlapping regions). If something might call memcpy() with overlapping or identical regions, it should use memmove().
lib/string.c void * memmove(void * dest,const void *src,size_t count) · line 506
Quoted 11 lines
lib/string.c +3
-0/* while all data is aligned (common case), copy a word at a time */ if ( (((ulong )dest | (ulong )src)& (sizeof (*dl) -1 )) ==0 ) {while (count>=sizeof (*dl)) {@@ -497,6 +500,9 @@ void * memmove(void * dest,const void *src,size_t count) {char *tmp, *s; +if (src == dest) +return dest; +if (dest<= src) {
Here it is ok, but the check <= could be modified to < too.
Just to clarify my reasoning: the only reason why memcpy() exists, is because it should have been a faster version of memmove() without the necessary checks.
So if a bug proof of version of memcpy() is wanted, there is no need to have a different implementation for memcpy() and memcpy() could just be an alias for memmove().
But adding a check for identity to memcpy() is unnecessary.
Sorry, but I had to comment this after having read to many comments in a bug about something similiar in
https://bugzilla.redhat.com/show_bug.cgi?id=638477
(Be aware, reading that bug might hurt your brain)
Regards,
Alexander
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Dear Alexander Holler,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 7 lines
lib/string.c +0
-0@@ -467,6 +467,9 @@ void * memcpy(void *dest, const void *src, size_t count)unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;char *d8, *s8;
Quoted 9 lines
lib/string.c void * memcpy(void *dest, const void *src, size_t count) · line 472
Quoted 3 lines
lib/string.c +3
-0+if (src == dest) +return dest; +here is the same, as in the patch I've commented before. There exist no reason to add a check for identity to
memcpy()because memcpy doesn't support overlapping regions (and identity is just a special case of overlapping regions). If something might callmemcpy()with overlapping or identical regions, it should usememmove().
In an ideal world, nobody will ever use any interfces in a non-compliant or incorrect way.
In reality, all kind of errors happen. A little defensive programming like the one above helps a lot, so please stop complaining even if you think you don't need this.
Thanks.
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Mirrors should reflect a little before throwing back images. - Jean Cocteau
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Am 23.05.2011 23:55, schrieb Wolfgang Denk:
Quoted 20 lines
Dear Alexander Holler,
Quoted 7 lines · In message<[email protected]> you wrote:
Quoted 7 lines
lib/string.c +0
-0@@ -467,6 +467,9 @@ void * memcpy(void *dest, const void *src, size_t count)unsigned long *dl = (unsigned long *)dest, *sl = (unsigned long *)src;char *d8, *s8;Quoted 9 lines
lib/string.c void * memcpy(void *dest, const void *src, size_t count) · line 472
Quoted 3 lines
lib/string.c +3
-0+if (src == dest) +return dest; +here is the same, as in the patch I've commented before. There exist no reason to add a check for identity to
memcpy()because memcpy doesn't support overlapping regions (and identity is just a special case of overlapping regions). If something might callmemcpy()with overlapping or identical regions, it should usememmove().
Quoted 6 lines
In an ideal world, nobody will ever use any interfces in a non-compliant or incorrect way.
In reality, all kind of errors happen. A little defensive programming like the one above helps a lot, so please stop complaining even if you think you don't need this.
So you I will look forward to checks for NULL pointers and similiar in all C standard functions implemented in u-boot to circumvent tons of possible real world bugs in all callers of strcpy, strlen, mem* and whatever.
Reads promising,
regards,
Alexander
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Dear Alexander Holler,
Quoted 5 lines · In message <[email protected]> you wrote:
So you I will look forward to checks for NULL pointers and similiar in all C standard functions implemented in u-boot to circumvent tons of possible real world bugs in all callers of strcpy, strlen, mem* and whatever.
If you think a bit about this, you may find it more difficult than you expect. Keep in mind that on most systems supported by U-Boot code like
int *p = (int *)0;
print("*p = %d\n", *p);is perfectly legal and supposed to work without any problems - because 0 is a legal address, and it makes perfect senze that commands like "md" or "cp" can be used to access it. In the result, strcpy(), strlen(), mem*() and whatever must beable to work on address 0 likeon any other address, too.
:-P
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Minds are like parachutes - they only function when open.
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Am 24.05.2011 00:22, schrieb Wolfgang Denk:
Quoted 17 lines
Dear Alexander Holler,
Quoted 5 lines · In message<[email protected]> you wrote:
So you I will look forward to checks for NULL pointers and similiar in all C standard functions implemented in u-boot to circumvent tons of possible real world bugs in all callers of strcpy, strlen, mem* and whatever.
If you think a bit about this, you may find it more difficult than you expect. Keep in mind that on most systems supported by U-Boot code like
int *p = (int *)0; print("*p = %d\n", *p);
Quoted 7 lines
is perfectly legal and supposed to work without any problems - because 0 is a legal address, and it makes perfect senze that commands like "md" or "cp" can be used to access it. In the result,
strcpy(),strlen(), mem*() and whatever must beable to work on address 0 likeon any other address, too.:-P
I've never seen a valid use of strcpy() with a null-pointer in real world programs, which we are talking about, except in bugs.
BTW, you missed to quote my suggestion to get rid of the implementation of memcpy() and use always memmove(). That would be really defensive programming and if the unnecessary identity-check in memcpy isn't of interest, the additional other check done by memmove() shouldn't be a problem too.
But I will stop complaining as requested and getting silent again.
Regards,
Alexander
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Quoted 15 lines · On Monday, May 23, 2011 18:38:49 Alexander Holler wrote:
Am 24.05.2011 00:22, schrieb Wolfgang Denk:
Quoted 14 lines
Quoted 4 lines · Alexander Holler
So you I will look forward to checks for NULL pointers and similiar in all C standard functions implemented in u-boot to circumvent tons of possible real world bugs in all callers of strcpy, strlen, mem* and whatever.
If you think a bit about this, you may find it more difficult than you expect. Keep in mind that on most systems supported by U-Boot code like
int *p = (int *)0; print("*p = %d\n", *p);
Quoted 8 lines
Quoted 5 lines
is perfectly legal and supposed to work without any problems - because 0 is a legal address, and it makes perfect senze that commands like "md" or "cp" can be used to access it. In the result,
strcpy(),strlen(), mem*() and whatever must beable to work on address 0 likeon any other address, too.I've never seen a valid use of
strcpy()with a null-pointer in real world programs, which we are talking about, except in bugs.
i'm lazy and type "0" all the time for loading files, copying memory, displaying things, etc... in u-boot. i dont feel like retraining my finger muscle memory if i dont have to ;).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110523/8f2a1b98/attachment.pgp
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Am 24.05.2011 05:47, schrieb Mike Frysinger:
Quoted 7 lines
Quoted 2 lines
I've never seen a valid use of
strcpy()with a null-pointer in real world programs, which we are talking about, except in bugs.i'm lazy and type "0" all the time for loading files, copying memory, displaying things, etc... in u-boot. i dont feel like retraining my finger muscle memory if i dont have to ;).
-mike
Using a 4 should be better for your finger.
Alexander
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
In reply to Wolfgang DenkQuoted 13 lines · On Tue, 24 May 2011 00:22:49 +0200 Wolfgang Denk <[email protected]> wrote:
Dear Alexander Holler,
Quoted 5 lines · In message <[email protected]> you wrote:
So you I will look forward to checks for NULL pointers and similiar in all C standard functions implemented in u-boot to circumvent tons of possible real world bugs in all callers of strcpy, strlen, mem* and whatever.
If you think a bit about this, you may find it more difficult than you expect. Keep in mind that on most systems supported by U-Boot code like
Quoted 5 lines
int *p = (int *)0; print("*p = %d\n", *p);is perfectly legal and supposed to work without any problems -
Might want to pass -fno-delete-null-pointer-checks...
As for memcpy/memmove, if in U-Boot it's to be legal to pass overlapping regions to memcpy(), why have separate implementations (not to mention bcopy...)?
-Scott
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Dear Scott Wood,
Quoted 2 lines · In message <[email protected]> you wrote:
Might want to pass -fno-delete-null-pointer-checks...
Thanks for pointing out.
Quoted 3 lines
As for memcpy/memmove, if in U-Boot it's to be legal to pass overlapping regions to
memcpy(), why have separate implementations (not to mention bcopy...)?
Define "legal"? Legal as in "everything that is not explicitly forbidden"? Legal as in "any code that does not exwecute the HCF instruction"?
The man page says: "The memory areas should not overlap. Use memmove(3) if the memory areas do overlap." I have nothing to add here.
As for the different implementations: well, U-Boot imported tons of code from different sources, using different interfaces and library functions. Where needed, the missing library functions got added.
Cleanup patches are always welcome.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Hegel was right when he said that we learn from history that man can never learn anything from history. - George Bernard Shaw
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
In reply to Matthias WeisserHello Wolfgang
Am 23.05.2011 11:03, schrieb Matthias Weisser:
Quoted 3 lines
In some cases (e.g. bootm with a elf payload which is already at the right position) there is a in place copy of data to the same address. Catching this saves some ms while booting.
What about this patch? As the initial submission of the patch was inside the merge window (see http://patchwork.ozlabs.org/patch/90725/) I would like to see the current version of this patch (see
http://patchwork.ozlabs.org/patch/96841/) in the upcoming release.
Sorry for the broken reference in patchwork. I try my best next time.
If the community decides to NACK the patch I would be happy if you could accept http://patchwork.ozlabs.org/patch/79325/
Thanks
Matthias
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
Am 14.06.2011 08:18, schrieb Matthias Wei?er:
Quoted 12 lines
Hello Wolfgang
Am 23.05.2011 11:03, schrieb Matthias Weisser:
Quoted 3 lines
In some cases (e.g. bootm with a elf payload which is already at the right position) there is a in place copy of data to the same address. Catching this saves some ms while booting.
What about this patch? As the initial submission of the patch was inside the merge window (see http://patchwork.ozlabs.org/patch/90725/) I would like to see the current version of this patch (see
http://patchwork.ozlabs.org/patch/96841/) in the upcoming release.
Quoted 4 lines
Sorry for the broken reference in patchwork. I try my best next time.
If the community decides to NACK the patch I would be happy if you could accept http://patchwork.ozlabs.org/patch/79325/
Ping?
Thanks
Matthias
PATCH v2 1/1 memcpy/memmove: Do not copy to same address
In reply to Matthias WeißerDear Wolfgang
Am 14.06.2011 08:18, schrieb Matthias Wei?er:
Quoted 9 lines
Am 23.05.2011 11:03, schrieb Matthias Weisser:
Quoted 3 lines
In some cases (e.g. bootm with a elf payload which is already at the right position) there is a in place copy of data to the same address. Catching this saves some ms while booting.
What about this patch? As the initial submission of the patch was inside the merge window (see http://patchwork.ozlabs.org/patch/90725/) I would like to see the current version of this patch (see
http://patchwork.ozlabs.org/patch/96841/) in the upcoming release.
May I kindly ask you if http://patchwork.ozlabs.org/patch/96841/ can go in as the merge window is now open again?
Thanks
Matthias
Wolfgang Denk
AppliedPATCH v2 1/1 memcpy/memmove: Do not copy to same address
In reply to Matthias WeisserDear Matthias Weisser,
Quoted 4 lines · In message <[email protected]> you wrote:
In some cases (e.g. bootm with a elf payload which is already at the right position) there is a in place copy of data to the same address. Catching this saves some ms while booting.
Quoted 8 lines
Signed-off-by Matthias Weisser <[email protected]>---
Changes since V1:
- Made subject more informative
- Removed the optimization from bcopy as bcopy is not used anywherelib/string.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The optimum committee has no members.
- Norman Augustine