|
|
|
runtime: fix memclr and impove memcopy.
Patch Set 1 #Patch Set 2 : diff -r a4c2e46af9b2 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r a4c2e46af9b2 https://go.googlecode.com/hg/ #
Total messages: 13
|
zhai
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
|
14 years, 5 months ago (2011年07月21日 12:26:18 UTC) #1 | ||||||||||||||||||||||||||||||||
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Have you seen a bug that this fixes?
On Fri, Jul 22, 2011 at 1:44 PM, <r@golang.org> wrote: > Have you seen a bug that this fixes? Please look at the memclr. if argument count == 1, it clear 8 bytes on amd64, or 4 bytes on 386. Not sure it's intend to do so.
On 22/07/2011, at 6:20 PM, zhai wrote: > On Fri, Jul 22, 2011 at 1:44 PM, <r@golang.org> wrote: > Have you seen a bug that this fixes? > Please look at the memclr. if argument count == 1, it clear 8 bytes on amd64, or 4 bytes on 386. > Not sure it's intend to do so. If you're not sure, why do you want to fix it? -rob
Let me put it another way. Do you have a test that fails before this fix and succeeds after it? Semantic changes to core libraries are not lightly made. -rob
On Fri, Jul 22, 2011 at 6:20 PM, zhai <qyzhai@gmail.com> wrote: > Please look at the memclr. if argument count == 1, it clear 8 bytes on > amd64, or 4 bytes on 386. In addition to Rob's comments, I think you've got this around the wrong way. The current amd64 code has MOVQ 16(SP), CX // puts the count in CX SHRQ 3,ドル CX // count >> 3 That means that, for count=1, CX will hold zero, so the "REP; STOSQ" will not write anything. In effect, the existing memclr under-clears but never over-clears. Dave.
On Fri, Jul 22, 2011 at 6:13 PM, Rob 'Commander' Pike <r@google.com> wrote: > Let me put it another way. Do you have a test that fails before this fix > and succeeds after it? Semantic changes to core libraries are not lightly > made. I have not found any tests fails before this "fix", the memset(memclr) is just a little not reasonable to me,I found it some days ago, and hesitate a while before send a changeset. I hope it will avoid some bugs in the future. sorry for my poor english.
> > If you're not sure, why do you want to fix it? > > -rob > I mean "is it intended to do so?"
the existing code overclears. it clears up to the next word boundary. but it is only called from places where that is okay. this CL replaces a C byte-clearing loop in a context where that is not okay with a call to memclr, so it also fixes memclr not to overclear, or else the switch would not be okay. everything about this CL looks good to me.
> > so it also fixes memclr > not to overclear, or else the switch would > not be okay. > It still pass all tests without changing the memclr asm code on 386.
Oh, duh. Somehow I completely missed the ADDQ 7,ドル CX Yes, the existing one does overclear. Serves me right for reading assembly code after midnight. Dave.
LGTM Don't seem to be any more objections, so submitting...
*** Submitted as http://code.google.com/p/go/source/detail?r=79cb8647dd5a *** runtime: replace byte-at-a-time zeroing loop with memclr R=golang-dev, r, r, dsymonds, rsc CC=golang-dev http://codereview.appspot.com/4813043 Committer: Russ Cox <rsc@golang.org>