Issue 7301062: code review 7301062: runtime: restrict stack root scan to locals and arguments (Closed)
Descriptionruntime: restrict stack root scan to locals and arguments
Patch Set 1 #Patch Set 2 : diff -r 835db789f270 https://code.google.com/p/go/ #Patch Set 3 : diff -r 835db789f270 https://code.google.com/p/go/ #
Total comments: 13
Patch Set 4 : diff -r 9d73a2fabea7 https://code.google.com/p/go/ #
Total comments: 12
Patch Set 5 : diff -r 9d73a2fabea7 https://code.google.com/p/go/ #Patch Set 6 : diff -r 9d73a2fabea7 https://code.google.com/p/go/ #Patch Set 7 : diff -r 17a869d2ae24 https://code.google.com/p/go/ #Patch Set 8 : diff -r 0e1b7425101a https://code.google.com/p/go/ #
Total comments: 3
MessagesTotal messages: 32
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
This change causes the garbage collector to scan somewhat more of the open frame than is needed. A forthcoming change will populate the locals information in the Func structure, allowing the scan to be truly limited to the locals area.
Sign in to reply to this message.
Hello, Thank you for this proposal. How does this change interact with the ongoing work by atom, and the outstanding change to the arm traceback code by minux https://codereview.appspot.com/7299055/ ? Cheers Dave
Sign in to reply to this message.
For the most part this CL's changes to the stack trace code are trying to refactor things so that it is a bit easier to use from different contexts. Until now we've just kept adding flags for every mode we need. Probably it needs to incorporate Minux's last change or two. Carl has just started working on Go, and for the most part he's been working on getting familiar with the code base. The short-term plan is to make stack collections precise, so that only live pointer slots are considered. (That should obsolete the -Z linker option.) Atom is working on making heap collections precise, so that data structures are walked correctly. The two efforts should complement each other nicely. Now that he's mostly up to speed, Carl's going to circulate a design sketch for his planned work soon. (The long-term plan is to make the garbage collector somewhat incremental, but that's still a ways off.) For what it's worth, we also have someone working on getting up to speed on scheduling, so that maybe we can eliminate the difference between blocking and non-blocking. But there's nothing even approaching a design for that yet. Russ
Sign in to reply to this message.
That is excellent news. Welcome Carl! On Fri, Feb 8, 2013 at 10:52 AM, Russ Cox <rsc@golang.org> wrote: > For the most part this CL's changes to the stack trace code are trying to > refactor things so that it is a bit easier to use from different contexts. > Until now we've just kept adding flags for every mode we need. Probably it > needs to incorporate Minux's last change or two. > > Carl has just started working on Go, and for the most part he's been working > on getting familiar with the code base. The short-term plan is to make stack > collections precise, so that only live pointer slots are considered. (That > should obsolete the -Z linker option.) Atom is working on making heap > collections precise, so that data structures are walked correctly. The two > efforts should complement each other nicely. Now that he's mostly up to > speed, Carl's going to circulate a design sketch for his planned work soon. > (The long-term plan is to make the garbage collector somewhat incremental, > but that's still a ways off.) > > For what it's worth, we also have someone working on getting up to speed on > scheduling, so that maybe we can eliminate the difference between blocking > and non-blocking. But there's nothing even approaching a design for that > yet. > > Russ
Sign in to reply to this message.
https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/proc.c#newco... src/pkg/runtime/proc.c:1006: entersyscall(int32 dummy) Name this void ·entersyscall(int32 dummy) This takes advantage of the fact that a leading · is shorthand for "the current package". The C compiler doesn't know this, though, so it won't complain about the prototype not matching the function, which is all we care about. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/proc.c#newco... src/pkg/runtime/proc.c:1062: runtime·entersyscall() and then you can delete this function. Not having the extra call is important: we want to save the information for runtime.entersyscall's caller. The extra frame makes the new code not different from the old code. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_arm.c File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_ar... src/pkg/runtime/traceback_arm.c:209: runtime·walkstack(byte *pc0, byte *sp, byte *lr0, G *gp, void (*fn)(Func*, byte*, byte*, void*), void *arg) Can you make gentraceback use this? It would be good not to have two copies. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_ar... src/pkg/runtime/traceback_arm.c:291: runtime·printf("Unknown PC %p\n", pc); This seems to me quite likely to happen, so it should be runtime.throw. Using throw will also take care of putting a good prefix on the message so that it is clear where the error is coming from. Right now, if a program just prints Unknown PC 0x1234 and then dies with whatever signal a breakpoint generates, it will be very confusing to end users. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:208: runtime·walkstack(byte *pc0, byte *sp, byte *lr0, G *gp, void (*fn)(Func*, byte*, byte*, void*), void *arg) Same comment about making gentraceback use this. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:278: runtime·printf("Unknown PC %p\n", pc); Same comment.
Sign in to reply to this message.
https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_arm.c File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_ar... src/pkg/runtime/traceback_arm.c:209: runtime·walkstack(byte *pc0, byte *sp, byte *lr0, G *gp, void (*fn)(Func*, byte*, byte*, void*), void *arg) walkstack assumes the stack is parse-able. gentraceback is best effort. I am concerned that walkstack might not be right for producing stack traces when the stack is corrupt or during the sigprof handler. If you think this is okay, I will merge the two functions. I think we can use "fn == nil" as a hint to behave more like genstacktrace. I am not sure about sigprof (a "pcbuf != nil" case).
Sign in to reply to this message.
I agree that walkstack needs to not crash during a crash just because it can't understand the stack, but that should be easy to do with a flag.
Sign in to reply to this message.
Of course. Is it safe to assume the stack is parse-able during a sigprof handler?
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 2:55 PM, <cshapiro@golang.org> wrote: > Of course. Is it safe to assume the stack is parse-able during a > sigprof handler? > I think all the stack traversals should be best effort (not abort, just stop) unless there is a very good reason they need to complete. Garbage collection is a very good reason, but I can't think of any others. Dropping a profiling entry is not a big deal. Russ
Sign in to reply to this message.
PTAL https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/proc.c#newco... src/pkg/runtime/proc.c:1006: entersyscall(int32 dummy) On 2013/02/14 20:59:59, rsc wrote: > Name this > void > ·entersyscall(int32 dummy) > > This takes advantage of the fact that a leading · is shorthand for "the current > package". The C compiler doesn't know this, though, so it won't complain about > the prototype not matching the function, which is all we care about. > Done. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/proc.c#newco... src/pkg/runtime/proc.c:1062: runtime·entersyscall() On 2013/02/14 20:59:59, rsc wrote: > and then you can delete this function. Not having the extra call is important: > we want to save the information for runtime.entersyscall's caller. The extra > frame makes the new code not different from the old code. > Done. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_arm.c File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_ar... src/pkg/runtime/traceback_arm.c:209: runtime·walkstack(byte *pc0, byte *sp, byte *lr0, G *gp, void (*fn)(Func*, byte*, byte*, void*), void *arg) Done. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_ar... src/pkg/runtime/traceback_arm.c:291: runtime·printf("Unknown PC %p\n", pc); I have replaced the abort with a throw. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:208: runtime·walkstack(byte *pc0, byte *sp, byte *lr0, G *gp, void (*fn)(Func*, byte*, byte*, void*), void *arg) Done. https://codereview.appspot.com/7301062/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:278: runtime·printf("Unknown PC %p\n", pc); Done.
Sign in to reply to this message.
LGTM after fixes https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/asm_amd64.s... src/pkg/runtime/asm_amd64.s:716: TEXT runtime·abort(SB),7,$0 should be able to revert this file now. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:1266: pc = (byte*)*((uintptr*)sp - 1); I believe this is wrong for ARM (it's one frame off). Better to add a gcpc field to the G and then set it wherever gcsp is set. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/runtime.h#n... src/pkg/runtime/runtime.h:708: void runtime·abort(void); delete https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/softfloat_a... File src/pkg/runtime/softfloat_arm.c (left): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/softfloat_a... src/pkg/runtime/softfloat_arm.c:17: void runtime·abort(void); should be able to revert this file now. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/traceback_a... File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/traceback_a... src/pkg/runtime/traceback_arm.c:144: if (fn != nil && pc == (uintptr)runtime·goexit) s/fn != nil && // ? https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/vlrt_386.c File src/pkg/runtime/vlrt_386.c (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/vlrt_386.c#... src/pkg/runtime/vlrt_386.c:33: // declared here to avoid include of runtime.h can revert this file. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/vlrt_arm.c File src/pkg/runtime/vlrt_arm.c (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/vlrt_arm.c#... src/pkg/runtime/vlrt_arm.c:27: void runtime·abort(void); can revert this file.
Sign in to reply to this message.
https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/asm_amd64.s... src/pkg/runtime/asm_amd64.s:716: TEXT runtime·abort(SB),7,$0 Okay. I think this is generally useful and makes the error handling facilities more symmetric across all of the targets. I can provide this feature in a separate change. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:1266: pc = (byte*)*((uintptr*)sp - 1); Right. Done. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/runtime.h#n... src/pkg/runtime/runtime.h:708: void runtime·abort(void); Done. https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/traceback_a... File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/traceback_a... src/pkg/runtime/traceback_arm.c:144: if (fn != nil && pc == (uintptr)runtime·goexit) Okay.
Sign in to reply to this message.
https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/7301062/diff/16001/src/pkg/runtime/asm_amd64.s... src/pkg/runtime/asm_amd64.s:716: TEXT runtime·abort(SB),7,$0 On 2013/02/27 21:56:20, cshapiro wrote: > Okay. I think this is generally useful and makes the error handling facilities > more symmetric across all of the targets. The use of abort at all is only in leftover C library support code from Plan 9. Go runtime code should not be calling abort, so it should not exist where not needed. In particular, it must not be used for shipped code that users might trip over. If you are debugging and want to make the code stop for a breakpoint, you can already call runtime.breakpoint().
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=9742f722b558 *** runtime: restrict stack root scan to locals and arguments R=rsc CC=golang-dev https://codereview.appspot.com/7301062
Sign in to reply to this message.
On Mon, Mar 4, 2013 at 7:50 PM, <cshapiro@golang.org> wrote: > *** Submitted as > https://code.google.com/p/go/**source/detail?r=9742f722b558<https://code.goog... > > runtime: restrict stack root scan to locals and arguments > Did you find the 386 bug you were running into? What did it turn out to be? Russ
Sign in to reply to this message.
Seems to break arm: fatal error: unknown pc goroutine 1 [running]: [fp=0x1045d1f8] runtime.throw(0x2ff8a4) /usr/local/go/src/pkg/runtime/panic.c:465 +0x50 [fp=0x1045d238] runtime.gentraceback(0x6098c, 0x1045d29c, 0x0, 0x1043f090, 0x0, ...) /usr/local/go/src/pkg/runtime/traceback_arm.c:71 +0x16c [fp=0x1045d270] addstackroots(0x1043f090) /usr/local/go/src/pkg/runtime/mgc0.c:1348 +0x80 [fp=0x1045d298] addroots() /usr/local/go/src/pkg/runtime/mgc0.c:1409 +0x258 [fp=0x1045d3d8] gc(0xb6b50dec) /usr/local/go/src/pkg/runtime/mgc0.c:1851 +0x1e8 ----- stack segment boundary ----- [fp=0xb6b50df4] runtime.gc(0x0) /usr/local/go/src/pkg/runtime/mgc0.c:1789 +0xf0 [fp=0xb6b50e24] runtime.mallocgc(0x20, 0x0, 0x1, 0x1) /usr/local/go/src/pkg/runtime/zmalloc_linux_arm.c:101 +0x1c4 [fp=0xb6b50e38] runtime.mal(0x14) /usr/local/go/src/pkg/runtime/zmalloc_linux_arm.c:611 +0x3c [fp=0xb6b50e6c] runtime.makemap_c(0x18e674, 0x0, 0x0) /usr/local/go/src/pkg/runtime/hashmap.c:826 +0xe8 [fp=0xb6b50e7c] runtime.makemap(0x18e674, 0x0, 0x0, 0xb6fb528c) /usr/local/go/src/pkg/runtime/hashmap.c:873 +0x38 [fp=0xb6b50e94] unicode.init() /usr/local/go/src/pkg/unicode/tables.go:48 +0x88 [fp=0xb6b50ea0] go/parser.init() /usr/local/go/src/pkg/go/parser/parser.go:2410 +0x70 [fp=0xb6b50fb8] main.init() /usr/local/go/src/cmd/go/vet.go:37 +0x78 [fp=0xb6b50fd0] runtime.main() /usr/local/go/src/pkg/runtime/proc.c:179 +0x80 [fp=0xb6b50fd0] runtime.goexit() /usr/local/go/src/pkg/runtime/proc.c:1151
Sign in to reply to this message.
Message was sent while issue was closed.
I found the first problem for this CL with ARM, but that just led to a second problem. Rolling back in CL 7493044 so that we can get back to a working ARM toolchain. Please try to debug on ARM.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/7301062/diff/37001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/7301062/diff/37001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:1320: sp = (byte*)&gp; This should be sp = runtime.getcallersp(&gp) in order to work on ARM. However, this is not the only thing wrong with this CL on ARM, and I don't know what the other problems are.
Sign in to reply to this message.
On Mon, Mar 4, 2013 at 8:52 PM, Russ Cox <rsc@golang.org> wrote: > Did you find the 386 bug you were running into? What did it turn out to be? > Yes. When a goroutine has been created but has not been started, gentrackback pretends that gp->fnstart->fn is the PC for the topmost frame and uses the SP of the fake goexit frame. This can cause the garbage collector to over-scan the stack as it assumes the PC corresponds to a function which has executed its prologue and adjusted the SP down some number of frame bytes. The resolution is to not unwind the stack in this case and only scan the incoming arguments area for the un-started goroutine. I added a comment that describes this condition.
Sign in to reply to this message.
On Tue, Mar 5, 2013 at 1:38 PM, Carl Shapiro <cshapiro@golang.org> wrote: > On Mon, Mar 4, 2013 at 8:52 PM, Russ Cox <rsc@golang.org> wrote: > >> Did you find the 386 bug you were running into? What did it turn out to >> be? >> > > Yes. > > When a goroutine has been created but has not been started, gentrackback > pretends that gp->fnstart->fn is the PC for the topmost frame and uses the > SP of the fake goexit frame. This can cause the garbage collector to > over-scan the stack as it assumes the PC corresponds to a function which > has executed its prologue and adjusted the SP down some number of frame > bytes. > Great. I found the place you handle this in the garbage collector, but I don't understand why the traceback routines don't work (and perhaps that's relevant to ARM, since writing special case code like this will work on x86 but be one word off on ARM). The traceback_x86.c code says: 79 // Found an actual function. 80 if(fp == nil) { 81 fp = sp; 82 if(pc > f->entry && f->frame >= sizeof(uintptr)) 83 fp += f->frame - sizeof(uintptr); 84 if(lr == 0) 85 lr = *(uintptr*)fp; 86 fp += sizeof(uintptr); 87 } else if(lr == 0) 88 lr = *(uintptr*)fp; The pc > f->entry test should handle the case where the function has not yet started executing. That is, the traceback does not assume that the PC corresponds to a function which has executed its prologue and adjusted the SP down some number of frame bytes. If at all possible I think we should try to eliminate the special case in the garbage collector, make sure the traceback routine is correct, and use it. Russ >
Sign in to reply to this message.
Message was sent while issue was closed.
After your pointer to the argument calculations, I made the changes below on my ARM machine. I believe they are required for correctness on ARM, but they are not sufficient: I still get the "invalid freelist" crash. https://codereview.appspot.com/7301062/diff/37001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/7301062/diff/37001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:1308: addroot((Obj){sp + f->frame, f->args, 0}); I changed this to sp + 4 + f->frame for ARM. https://codereview.appspot.com/7301062/diff/37001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:1344: addroot((Obj){sp, f->args, 0}); I changed this to sp+4 for ARM.
Sign in to reply to this message.
Another possibility is that this fails because only functions that can split the stack have valid f->args counts. Functions that don't split the stack often have no recorded argument size, because only the stack split code used to care. Russ
Sign in to reply to this message.
On Tue, Mar 5, 2013 at 2:35 PM, Russ Cox <rsc@golang.org> wrote: > Another possibility is that this fails because only functions that can > split the stack have valid f->args counts. > Functions that don't split the stack often have no recorded argument size, > because only the stack split > code used to care. > This is an interesting observation. The change I submitted scans (f->frame - sizeof(uintptr)) bytes above the SP plus f->args bytes above the return address. The arguments area is effectively scanned twice, once in the callee and again in the caller. If, instead of using f->frame, I use f->locals so the arguments are only scanned in the callee I can induce a free-list corruption on 386 and amd64. I was unaware of the invalid f->args issue. I would like to correct that. What needs to be done to emit valid .args information for all functions?
Sign in to reply to this message.
On Tue, Mar 5, 2013 at 3:02 PM, Carl Shapiro <cshapiro@golang.org> wrote: > On Tue, Mar 5, 2013 at 2:35 PM, Russ Cox <rsc@golang.org> wrote: > >> Another possibility is that this fails because only functions that can >> split the stack have valid f->args counts. >> Functions that don't split the stack often have no recorded argument >> size, because only the stack split >> code used to care. >> > > This is an interesting observation. > > The change I submitted scans (f->frame - sizeof(uintptr)) bytes above the > SP plus f->args bytes above the return address. The arguments area is > effectively scanned twice, once in the callee and again in the caller. If, > instead of using f->frame, I use f->locals so the arguments are only > scanned in the callee I can induce a free-list corruption on 386 and amd64. > I wonder why this still broke on arm, then. There must still be something wrong even when using f->frame. I was unaware of the invalid f->args issue. I would like to correct that. > What needs to be done to emit valid .args information for all functions? > You can run the go build command with -ldflags to pass flags to the linker. The -a flag will make the linker print the full assembly for the binary being put together. If you grep the assembly for TEXT lines with ,7, you will find the ones that have been flagged by hand as no stack split. (The linker flags some other automatically, but those will say ,2,.) Of those, you only care about the ones with no argument information ($0). Some of them really have no arguments; others might be lazy or special in some way. Those are the ones that need to be considered and possibly fixed. go build -ldflags -a cmd/godoc |grep 'TEXT.*,7,\$0$' If you run that you'll find that one big offender is automatically generated method wrappers. Those should be easy to fix in cmd/gc. go build -ldflags -a cmd/godoc |grep 'TEXT.*,7,\$0$' |grep -v '\.(\*' The remaining ones are mostly things written in assembly, and they could be fixed by hand. You could probably make the linker reject a function if it says the args size is n but refers to m(FP) for m >= n, and you'd find these pretty fast. There are always exceptions. The runtime.morestack family of functions really has no arguments, but you shouldn't need to consider it in tracebacks either. runtime.printf has arguments but you don't know how many. Polymorphic functions such as runtime.chansend have arguments that vary in size according to the initial type parameter. I suppose all of these need to be addressed in some way before this change is safe to apply. Apologies for not noticing that earlier in the review. Russ
Sign in to reply to this message.
2013/3/5 Russ Cox <rsc@golang.org>: > On Tue, Mar 5, 2013 at 3:02 PM, Carl Shapiro <cshapiro@golang.org> wrote: >> >> On Tue, Mar 5, 2013 at 2:35 PM, Russ Cox <rsc@golang.org> wrote: >>> >>> Another possibility is that this fails because only functions that can >>> split the stack have valid f->args counts. >>> Functions that don't split the stack often have no recorded argument >>> size, because only the stack split >>> code used to care. >> >> >> This is an interesting observation. >> >> The change I submitted scans (f->frame - sizeof(uintptr)) bytes above the >> SP plus f->args bytes above the return address. The arguments area is >> effectively scanned twice, once in the callee and again in the caller. If, >> instead of using f->frame, I use f->locals so the arguments are only scanned >> in the callee I can induce a free-list corruption on 386 and amd64. > > > I wonder why this still broke on arm, then. There must still be something > wrong even when using f->frame. I'm very skeptical of the runtime on ARM. Are there any lock-free datastructures in runtime or "missed" atomics (i.e. atomics that aren't using atomicload/atomicstore on ARM)? If so, I expect breakage, 5a/5l cannot generate memory fences (and ARM has a relaxed memory ordering, like Alpha). These will be necessary for the scheduler work I did, but I suspect they may be necessary elsewhere. It would cause inconsistent, unpredictable breakage. (Sorry Russ, I'm terrible at reply-all) --dho
Sign in to reply to this message.
On Tue, Mar 5, 2013 at 6:59 PM, Devon H. O'Dell <devon.odell@gmail.com>wrote: > > I wonder why this still broke on arm, then. There must still be something > > wrong even when using f->frame. > > I'm very skeptical of the runtime on ARM. Are there any lock-free > datastructures in runtime or "missed" atomics (i.e. atomics that > aren't using atomicload/atomicstore on ARM)? If so, I expect breakage, > 5a/5l cannot generate memory fences (and ARM has a relaxed memory > ordering, like Alpha). These will be necessary for the scheduler work > I did, but I suspect they may be necessary elsewhere. It would cause > inconsistent, unpredictable breakage. > There are certainly no atomics needed in this CL, and ARM seems pretty stable without this CL. That doesn't mean you're wrong, but I feel like we'd have seen other signs. Russ
Sign in to reply to this message.
On Wed, Mar 6, 2013 at 10:59 AM, Devon H. O'Dell <devon.odell@gmail.com> wrote: > I'm very skeptical of the runtime on ARM. Are there any lock-free > datastructures in runtime or "missed" atomics (i.e. atomics that > aren't using atomicload/atomicstore on ARM)? If so, I expect breakage, > 5a/5l cannot generate memory fences (and ARM has a relaxed memory > ordering, like Alpha). These will be necessary for the scheduler work > I did, but I suspect they may be necessary elsewhere. It would cause > inconsistent, unpredictable breakage. in theory, we need DMB even after cas, but note that we always call cas as a function and not as inline code. perhaps this fact makes the data race window very small and so we haven't encounter them (yet). i just got a somewhat powerful ARM machine with quad Cortex-A9 and 2GiB memory, I can run some extensive multithreaded tests like what fullung did for amd64 to see if this will cause any real problems.
Sign in to reply to this message.
2013/3/6 minux <minux.ma@gmail.com>: > On Wed, Mar 6, 2013 at 10:59 AM, Devon H. O'Dell <devon.odell@gmail.com> wrote: >> I'm very skeptical of the runtime on ARM. Are there any lock-free >> datastructures in runtime or "missed" atomics (i.e. atomics that >> aren't using atomicload/atomicstore on ARM)? If so, I expect breakage, >> 5a/5l cannot generate memory fences (and ARM has a relaxed memory >> ordering, like Alpha). These will be necessary for the scheduler work >> I did, but I suspect they may be necessary elsewhere. It would cause >> inconsistent, unpredictable breakage. > in theory, we need DMB even after cas, but note that we always call > cas as a function and not as inline code. perhaps this fact makes > the data race window very small and so we haven't encounter them (yet). > > i just got a somewhat powerful ARM machine with quad Cortex-A9 and > 2GiB memory, I can run some extensive multithreaded tests like what > fullung did for amd64 to see if this will cause any real problems. What machine is this? I have two different dual-core Cortex A9 boards running Android, so I should be able to get some testing going on as well. I wouldn't mind adding something more powerful to that mix since I'm also using them for Concurrency Kit. I think we have evidence that it causes problems already. The retry after the kernel-supplied CAS in the linux runtime·cas implementation I think is a bug that would be explained by this. --dho
Sign in to reply to this message.
On Thu, Mar 7, 2013 at 4:34 AM, Devon H. O'Dell <devon.odell@gmail.com> wrote: > 2013/3/6 minux <minux.ma@gmail.com>: >> i just got a somewhat powerful ARM machine with quad Cortex-A9 and >> 2GiB memory, I can run some extensive multithreaded tests like what >> fullung did for amd64 to see if this will cause any real problems. > > What machine is this? I have two different dual-core Cortex A9 boards > running Android, so I should be able to get some testing going on as > well. I wouldn't mind adding something more powerful to that mix since > I'm also using them for Concurrency Kit. odroid-u2 (note the power socket is very unusual, and i took me quite some time to find one suitable plug, and in the end i gave up and soldered the cord to the board directly; if you don't want to take the trouble, buy the official power adapter). > I think we have evidence that it causes problems already. The retry > after the kernel-supplied CAS in the linux runtime·cas implementation > I think is a bug that would be explained by this. i will take a closer look (i thought it's caused by kernel bug).
Sign in to reply to this message.
On Wed, Mar 6, 2013 at 3:34 PM, Devon H. O'Dell <devon.odell@gmail.com>wrote: > I think we have evidence that it causes problems already. The retry > after the kernel-supplied CAS in the linux runtime·cas implementation > I think is a bug that would be explained by this. > What evidence? What is "it"? The retry after the kernel-supplied CAS works around a kernel bug that was fixed in Linux 2.6.24. See the comment at the top of $GOROOT/src/pkg/sync/atomic/asm_linux_arm.s. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/ar...
Sign in to reply to this message.
2013/3/6 Russ Cox <rsc@golang.org>: > On Wed, Mar 6, 2013 at 3:34 PM, Devon H. O'Dell <devon.odell@gmail.com> > wrote: >> >> I think we have evidence that it causes problems already. The retry >> after the kernel-supplied CAS in the linux runtime·cas implementation >> I think is a bug that would be explained by this. > > > What evidence? What is "it"? > > The retry after the kernel-supplied CAS works around a kernel bug that was > fixed in Linux 2.6.24. > See the comment at the top of $GOROOT/src/pkg/sync/atomic/asm_linux_arm.s. > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/ar... Sorry, my misunderstanding. --dho
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
