|
|
Created:
11 years, 8 months ago by Ivan Krasin Modified:
10 years, 8 months ago CC:
iant, krasin, iant2, minux1, rsc, nigeltao, r, albert.strasheim, golang-dev Visibility:
Public. |
Descriptionruntime: add vdso support for linux/amd64. Fixes issue 1933.
Patch Set 1 #Patch Set 2 : diff -r 7a67d277c7e8 https://code.google.com/p/go #
Total comments: 6
Patch Set 3 : diff -r 7a67d277c7e8 https://code.google.com/p/go #Patch Set 4 : diff -r 7a67d277c7e8 https://code.google.com/p/go #Patch Set 5 : diff -r 7a67d277c7e8 https://code.google.com/p/go #Patch Set 6 : diff -r 7a67d277c7e8 https://code.google.com/p/go #Patch Set 7 : diff -r 7a67d277c7e8 https://code.google.com/p/go #Patch Set 8 : diff -r 7a67d277c7e8 https://code.google.com/p/go #Patch Set 9 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 10 : diff -r 3daca70c7743 https://code.google.com/p/go #
Total comments: 14
Patch Set 11 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 12 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 13 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 14 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 15 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 16 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 17 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 18 : diff -r 3daca70c7743 https://code.google.com/p/go #Patch Set 19 : diff -r 3daca70c7743 https://code.google.com/p/go #
Total comments: 8
Patch Set 20 : diff -r 441d5f62e9ff https://code.google.com/p/go #
Total comments: 6
Patch Set 21 : diff -r 5d3aaf0b717e https://code.google.com/p/go #Patch Set 22 : diff -r 5d3aaf0b717e https://code.google.com/p/go #Patch Set 23 : diff -r 5d3aaf0b717e https://code.google.com/p/go #Patch Set 24 : diff -r 5d3aaf0b717e https://code.google.com/p/go #
Total comments: 10
Patch Set 25 : diff -r 5d3aaf0b717e https://code.google.com/p/go #Patch Set 26 : diff -r 5d3aaf0b717e https://code.google.com/p/go #
Total comments: 2
Patch Set 27 : diff -r 5d3aaf0b717e https://code.google.com/p/go #Patch Set 28 : diff -r 5d3aaf0b717e https://code.google.com/p/go #Patch Set 29 : diff -r 87e3318f6a52 https://code.google.com/p/go #
Total comments: 22
Patch Set 30 : diff -r 9ef259879f24 https://code.google.com/p/go #Patch Set 31 : diff -r 9ef259879f24 https://code.google.com/p/go #Patch Set 32 : diff -r 9ef259879f24 https://code.google.com/p/go #Patch Set 33 : diff -r 9ef259879f24 https://code.google.com/p/go #Patch Set 34 : diff -r 9ef259879f24 https://code.google.com/p/go #
Total comments: 2
Patch Set 35 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 36 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 37 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 38 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 39 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 40 : diff -r 62f087306b18 https://code.google.com/p/go #
Total comments: 4
Patch Set 41 : diff -r 62f087306b18 https://code.google.com/p/go #
Total comments: 8
Patch Set 42 : diff -r 9bc8fafec2f5 https://code.google.com/p/go #Patch Set 43 : diff -r 9bc8fafec2f5 https://code.google.com/p/go #
Total comments: 4
Patch Set 44 : diff -r 391791277b74 https://code.google.com/p/go #Patch Set 45 : diff -r 391791277b74 https://code.google.com/p/go #
MessagesTotal messages: 56
http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:5: * This code is meant to be linked in to various programs that run on Linux. If you want to submit this to the Go repository, it needs to have the same three-line license that all the other files have. If this is somebody else's code, then they need to sign the copyright disclaimer, or it needs to be rewritten. http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:245: vdso_info.symstrings = 0; These values will all be zero anyhow. http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:290: /* That's all we need. */ We don't need or want to support arbitrary symbol lookups. It would be perfectly reasonable to look up all the symbols we expect to get from the VDSO right here. That may only be gettimeofday.
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:290: /* That's all we need. */ On 2012/07/26 16:48:33, iant wrote: > We don't need or want to support arbitrary symbol lookups. It would be > perfectly reasonable to look up all the symbols we expect to get from the VDSO > right here. That may only be gettimeofday. It's not actually too different in terms of a stack deficit. Do I understand correctly that there's no way to increase the stack size at this early stage?
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:290: /* That's all we need. */ > Do I understand correctly that there's no way to increase the stack size at this > early stage? At this early stage--before the program has really started--there is plenty of stack space, provided by the kernel. The limits on stack space only arise when we start making goroutines. Use #pragma textflag 7 to tell the function not to worry about stack space.
Sign in to reply to this message.
On 2012/07/26 17:32:49, iant wrote: > http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... > File src/pkg/runtime/vdso_linux_amd64.c (right): > > http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... > src/pkg/runtime/vdso_linux_amd64.c:290: /* That's all we need. */ > > Do I understand correctly that there's no way to increase the stack size at > this > > early stage? > > At this early stage--before the program has really started--there is plenty of > stack space, provided by the kernel. The limits on stack space only arise when > we start making goroutines. Use #pragma textflag 7 to tell the function not to > worry about stack space. I do set #pragma texflag 7, and I see the following compiler error (on slightly updated version of the code): rt0_amd64_linux: nosplit stack overflow 120 assumed on entry to _rt0_amd64_linux 112 on entry to runtime.linux_setup_vdso 88 after runtime.linux_setup_vdso uses 24 80 on entry to init_vdso_info 40 after init_vdso_info uses 40 32 on entry to vdso_init_from_sysinfo_ehdr -24 after vdso_init_from_sysinfo_ehdr uses 56 vdso_sym: nosplit stack overflow 120 assumed on entry to vdso_sym 64 after vdso_sym uses 56 56 on entry to vdso_match_version 8 after vdso_match_version uses 48 0 on entry to runtime.strcmp -16 after runtime.strcmp uses 16 runtime.linux_setup_vdso: nosplit stack overflow 120 assumed on entry to runtime.linux_setup_vdso 96 after runtime.linux_setup_vdso uses 24 88 on entry to init_vdso_info 48 after init_vdso_info uses 40 40 on entry to vdso_init_from_sysinfo_ehdr -16 after vdso_init_from_sysinfo_ehdr uses 56
Sign in to reply to this message.
also, I have actually seen the buffer underflow that caused a segfault.
Sign in to reply to this message.
Sorry, this is not something I know about.
Sign in to reply to this message.
On 2012/07/26 18:18:33, iant2 wrote: > Sorry, this is not something I know about. np, I will try to figure out it myself.
Sign in to reply to this message.
On 2012/07/26 18:09:46, krasin wrote: > I do set #pragma texflag 7, and I see the following compiler error (on slightly > updated version of the code): > > rt0_amd64_linux: nosplit stack overflow > 120 assumed on entry to _rt0_amd64_linux this is checked by 6l, which makes sure non-split stack don't use more memory than reserved for (we reserve some stack in each stack segment for these nosplit stack functions). maybe the limit is too low for 64-bit architectures, but from former experience, we tend not to increase the limit (but to workaround that). I think a better way would be, in the rt0 code, only backup the corresponding auxv for latter use, and then after we have initialized the split stack mechanism, do the actual initialization (perhaps when we first need to look up a symbol). ps: one easy way to fool the linker is to invoke function via function pointers.
Sign in to reply to this message.
On 2012/07/27 00:34:39, minux wrote: > On 2012/07/26 18:09:46, krasin wrote: > > I do set #pragma texflag 7, and I see the following compiler error (on > slightly > > updated version of the code): > > > > rt0_amd64_linux: nosplit stack overflow > > 120 assumed on entry to _rt0_amd64_linux > this is checked by 6l, which makes sure non-split stack don't use more memory > than reserved for (we reserve some stack in each stack segment for these > nosplit stack functions). > > maybe the limit is too low for 64-bit architectures, but from former experience, > we tend not to increase the limit (but to workaround that). > > I think a better way would be, in the rt0 code, only backup the corresponding > auxv for latter use, and then after we have initialized the split stack > mechanism, > do the actual initialization (perhaps when we first need to look up a symbol). > > ps: one easy way to fool the linker is to invoke function via function pointers. Actually, I have managed to produce the code that working within the current limit. I will upload once I have replaced spaces with tabs.
Sign in to reply to this message.
Hi Ian, please, take a first high-level look. Currently, this CL enables vdso-aware syscall.Time with an easy upgrade option for gettimeofday, getcpu, clock_gettime. question: I have used some definitions from /usr/include/elf.h. Most of them are also available in cmd/ld/elf.h. Should I just include cmd/ld/elf.h and simplify vdso_linux_amd64.c? krasin http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:245: vdso_info.symstrings = 0; On 2012/07/26 16:48:33, iant wrote: > These values will all be zero anyhow. Done.
Sign in to reply to this message.
Some comments. I wouldn't have code in runtime #include code in cmd/ld. They are compiled by different compilers--runtime by 6c, cmd/ld by gcc. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/rt0_linux_am... File src/pkg/runtime/rt0_linux_amd64.s (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/rt0_linux_am... src/pkg/runtime/rt0_linux_amd64.s:9: CALL runtime·linux_find_vdso_symbols(SB) Might as well make setup_vdso call find_vdso_symbols. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:1: #include "runtime.h" Needs copyright notice. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:5: #define PT_NULL 0 /* Program header table entry unused */ Formatting looks wrong--change tab to space? http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:15: #define DT_VERDEF 0x6ffffffc Formatting looks wrong. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:70: entry */ Put comment on one line. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:359: void runtime·linux_find_vdso_symbols(void) { I would suggest a different approach. There are only going to be a few symbols defined in the VDSO. Suppose you just walk through the dynamic symbol table and look for the ones we want. That saves dealing with the hash table entirely. http://codereview.appspot.com/6454046/diff/14001/src/pkg/syscall/asm_linux_am... File src/pkg/syscall/asm_linux_amd64.s (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/syscall/asm_linux_am... src/pkg/syscall/asm_linux_amd64.s:125: MOVQ runtime·__vdso_time_sym(SB), AX We also want to fix gettimeofday.
Sign in to reply to this message.
Ian, please, take another look. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/rt0_linux_am... File src/pkg/runtime/rt0_linux_amd64.s (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/rt0_linux_am... src/pkg/runtime/rt0_linux_amd64.s:9: CALL runtime·linux_find_vdso_symbols(SB) On 2012/07/27 17:24:23, iant wrote: > Might as well make setup_vdso call find_vdso_symbols. No, we can't. The stack will overflow. Remember, I only have 200 bytes here. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:1: #include "runtime.h" On 2012/07/27 17:24:23, iant wrote: > Needs copyright notice. Done. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:5: #define PT_NULL 0 /* Program header table entry unused */ On 2012/07/27 17:24:23, iant wrote: > Formatting looks wrong--change tab to space? Done. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:15: #define DT_VERDEF 0x6ffffffc On 2012/07/27 17:24:23, iant wrote: > Formatting looks wrong. Done. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:70: entry */ On 2012/07/27 17:24:23, iant wrote: > Put comment on one line. Done. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:359: void runtime·linux_find_vdso_symbols(void) { On 2012/07/27 17:24:23, iant wrote: > I would suggest a different approach. There are only going to be a few symbols > defined in the VDSO. Suppose you just walk through the dynamic symbol table and > look for the ones we want. That saves dealing with the hash table entirely. done. I have got rid of parsing hash table. http://codereview.appspot.com/6454046/diff/14001/src/pkg/syscall/asm_linux_am... File src/pkg/syscall/asm_linux_amd64.s (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/syscall/asm_linux_am... src/pkg/syscall/asm_linux_amd64.s:125: MOVQ runtime·__vdso_time_sym(SB), AX On 2012/07/27 17:24:23, iant wrote: > We also want to fix gettimeofday. Done. For some reason, VDSO version of gettimeofday works only 2x faster than the real syscall. This is reproduced on different kernels (3.2 and 3.5) and matches C version: #include <stdio.h> #include <sys/time.h> int main(void) { struct timeval tv; for (int i = 0; i < 1000000; i++) { gettimeofday(&tv, NULL); } }
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:131: static struct vdso_info I don't think this struct needs a name. It would be nice if we didn't have to make this static, but perhaps there is not enough stack space to do otherwise. http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:168: static symbol_key* cur_sym; As long as we have the vdso_info static struct, it seems to me we may as well put all the static variables in there. But I'm not sure we need cur_sym at all. http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:347: cur_sym = &sym_keys[VDSO_TIME_IND]; This isn't quite what I was expecting. What I was thinking is: walk through the symbol table once. For each symbol, check if we care about it. If we do, store the value. If you do that, I think you can drop the found_ver_ind static array. Just cache the current version, and update if the version index changes (which will never happen with the current VDSO).
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:131: static struct vdso_info On 2012/07/30 17:32:49, iant wrote: > I don't think this struct needs a name. > > It would be nice if we didn't have to make this static, but perhaps there is not > enough stack space to do otherwise. Done. http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:168: static symbol_key* cur_sym; On 2012/07/30 17:32:49, iant wrote: > As long as we have the vdso_info static struct, it seems to me we may as well > put all the static variables in there. But I'm not sure we need cur_sym at all. Done. http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:347: cur_sym = &sym_keys[VDSO_TIME_IND]; On 2012/07/30 17:32:49, iant wrote: > This isn't quite what I was expecting. What I was thinking is: walk through the > symbol table once. For each symbol, check if we care about it. If we do, store > the value. > > If you do that, I think you can drop the found_ver_ind static array. Just cache > the current version, and update if the version index changes (which will never > happen with the current VDSO). we do not have enough stack for that. I have introduced this complexity with found_ver_ind to save the stack. Currently, I literally use all the available stack. I am all for killing this ugliness, but based on our previous discussion, we don't have a way to get more stack.
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:347: cur_sym = &sym_keys[VDSO_TIME_IND]; On 2012/07/30 18:28:37, krasin wrote: > On 2012/07/30 17:32:49, iant wrote: > > This isn't quite what I was expecting. What I was thinking is: walk through > the > > symbol table once. For each symbol, check if we care about it. If we do, > store > > the value. > > > > If you do that, I think you can drop the found_ver_ind static array. Just > cache > > the current version, and update if the version index changes (which will never > > happen with the current VDSO). > we do not have enough stack for that. I have introduced this complexity with > found_ver_ind to save the stack. Currently, I literally use all the available > stack. > > I am all for killing this ugliness, but based on our previous discussion, we > don't have a way to get more stack. Well, forget about found_ver_ind. Still, you should be able to walk the symbol table once.
Sign in to reply to this message.
I don't know if you're still having trouble with the textflag 7 overflows, but if you are, an easy way around that would be to delay the code until later. It seems like you could put void (*sysargs)(int32, void*); in runtime.h, and then in runtime.c's runtime.args function do if(sysargs != nil) sysargs(argc, argv); and then in the Linux file write a handler and also void (*sysargs)(int32, void*) = linux_sysargs;
Sign in to reply to this message.
(Then the handler doesn't have to be textflag 7.)
Sign in to reply to this message.
On 2012/08/03 19:44:45, rsc wrote: > (Then the handler doesn't have to be textflag 7.) Thanks, Russ. I will try to do so.
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:111: Elf64_Xword d_val; /* Integer value */ Indent? http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:199: { Move the { to the end of the previous line. http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:325: { Ditto.
Sign in to reply to this message.
Please, take another look. http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:347: cur_sym = &sym_keys[VDSO_TIME_IND]; On 2012/07/30 19:49:21, iant wrote: > On 2012/07/30 18:28:37, krasin wrote: > > On 2012/07/30 17:32:49, iant wrote: > > > This isn't quite what I was expecting. What I was thinking is: walk through > > the > > > symbol table once. For each symbol, check if we care about it. If we do, > > store > > > the value. > > > > > > If you do that, I think you can drop the found_ver_ind static array. Just > > cache > > > the current version, and update if the version index changes (which will > never > > > happen with the current VDSO). > > we do not have enough stack for that. I have introduced this complexity with > > found_ver_ind to save the stack. Currently, I literally use all the available > > stack. > > > > I am all for killing this ugliness, but based on our previous discussion, we > > don't have a way to get more stack. > > > Well, forget about found_ver_ind. > > Still, you should be able to walk the symbol table once. Done. http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:111: Elf64_Xword d_val; /* Integer value */ On 2012/08/06 01:09:00, nigeltao wrote: > Indent? Done. http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:199: { On 2012/08/06 01:09:00, nigeltao wrote: > Move the { to the end of the previous line. Done. http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:325: { On 2012/08/06 01:09:00, nigeltao wrote: > Ditto. Done.
Sign in to reply to this message.
Thanks for working on this. Some more comments. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:165: { (byte*)"__vdso_time", 0xa33c485 }, These are the only symbols we care about. I don't see why we need the hash code. I just replace that with a pointer to the variable we want to set. Then just fill in that value when we find the symbol. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:313: uint32 name_hash = elf_hash(vdso_info.symstrings + sym->st_name); You are computing the hash of the symbol in order to see if we should compare the name. Don't bother. Just compare the name. Comparing the names is probably faster than computing the hash anyhow. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:319: if (runtime·strcmp(sym_key->name, vdso_info.symstrings + sym->st_name)) This is clearer if you use an explicit != 0. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:329: hash_sym_ptr[name_hash & HASH_SYM_MASK] = (void *)(vdso_info.load_offset + sym->st_value); If you store the address of the variable in sym_keys, and then set that variable here, you can remove hash_sym_ptr. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:386: vdso_info.cur_ver = &linux26; This indirection seems unnecessary.
Sign in to reply to this message.
PTAL http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:165: { (byte*)"__vdso_time", 0xa33c485 }, On 2012/08/08 22:10:50, iant wrote: > These are the only symbols we care about. I don't see why we need the hash > code. I just replace that with a pointer to the variable we want to set. Then > just fill in that value when we find the symbol. This hash code is used in hash_sym and is useful to locate a symbol while iterating through vdso symbol table. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:313: uint32 name_hash = elf_hash(vdso_info.symstrings + sym->st_name); On 2012/08/08 22:10:50, iant wrote: > You are computing the hash of the symbol in order to see if we should compare > the name. Don't bother. Just compare the name. Comparing the names is > probably faster than computing the hash anyhow. I need to compute the hash to locate sym_key which is used in runtime.strcmp(sym_key->name, ...). Once the hash is computed, it's faster to check it than to perform strcmp. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:319: if (runtime·strcmp(sym_key->name, vdso_info.symstrings + sym->st_name)) On 2012/08/08 22:10:50, iant wrote: > This is clearer if you use an explicit != 0. Done. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:329: hash_sym_ptr[name_hash & HASH_SYM_MASK] = (void *)(vdso_info.load_offset + sym->st_value); On 2012/08/08 22:10:50, iant wrote: > If you store the address of the variable in sym_keys, and then set that variable > here, you can remove hash_sym_ptr. Done. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:386: vdso_info.cur_ver = &linux26; On 2012/08/08 22:10:50, iant wrote: > This indirection seems unnecessary. Done. There's a good chance that multiple symbol versions will be provided by VDSO at some point (in a year or two). In this case, someone will have to reintroduce this indirection and don't forget about the stack limit.
Sign in to reply to this message.
On 2012/08/09 00:43:27, imkrasin wrote: > src/pkg/runtime/vdso_linux_amd64.c:165: { (byte*)"__vdso_time", 0xa33c485 }, > On 2012/08/08 22:10:50, iant wrote: > > These are the only symbols we care about. I don't see why we need the hash > > code. I just replace that with a pointer to the variable we want to set. > Then > > just fill in that value when we find the symbol. > > This hash code is used in hash_sym and is useful to locate a symbol while > iterating through vdso symbol table. But why do you need hash_sym at all? How does it help you? There are only four symbols here. Once you've got the symbol name from the symbol table, you can just check against each of the four symbols. And really we don't need all four symbols--there is no reason to look for a symbol that we won't call, and currently I think we only call two: gettimeofday and time. > There's a good chance that multiple symbol versions will be provided by VDSO at > some point (in a year or two). In this case, someone will have to reintroduce > this indirection and don't forget about the stack limit. As far as I can see we will always look for a specific version of the symbol. The symbol version should only change if the function changes in some incompatible way.
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:329: if (ver > MAX_VER_IND || found_ver_ind[ver] != ver_ind_marker) We only care about one version. I think you can drop the found_ver_ind array and just remember the one version we care about.
Sign in to reply to this message.
On 2012/08/09 00:54:26, iant wrote: > On 2012/08/09 00:43:27, imkrasin wrote: > > > src/pkg/runtime/vdso_linux_amd64.c:165: { (byte*)"__vdso_time", 0xa33c485 }, > > On 2012/08/08 22:10:50, iant wrote: > > > These are the only symbols we care about. I don't see why we need the hash > > > code. I just replace that with a pointer to the variable we want to set. > > Then > > > just fill in that value when we find the symbol. > > > > This hash code is used in hash_sym and is useful to locate a symbol while > > iterating through vdso symbol table. > > But why do you need hash_sym at all? How does it help you? There are only four > symbols here. Once you've got the symbol name from the symbol table, you can > just check against each of the four symbols. And really we don't need all four > symbols--there is no reason to look for a symbol that we won't call, and > currently I think we only call two: gettimeofday and time. Done. > > > > > There's a good chance that multiple symbol versions will be provided by VDSO > at > > some point (in a year or two). In this case, someone will have to reintroduce > > this indirection and don't forget about the stack limit. > > As far as I can see we will always look for a specific version of the symbol. > The symbol version should only change if the function changes in some > incompatible way. I mean that there will be added new vdso symbols (not time related) which will likely have linux3x version (just because they will be introduced in linux 3.x kernel) Although, keeping the code simple is fine.
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:329: if (ver > MAX_VER_IND || found_ver_ind[ver] != ver_ind_marker) On 2012/08/09 00:57:00, iant wrote: > We only care about one version. I think you can drop the found_ver_ind array > and just remember the one version we care about. Done.
Sign in to reply to this message.
OK, this is looking good. Thanks for your patience. Have you considered rsc's suggestion of delaying this code until later, to avoid the stack overflow problem? As far as I know nothing in the runtime calls these functions before setting up the stack, though it's certainly worth checking. Now, sorry about this, but there are a bunch of formatting nits. This stuff is trivial but it's useful to keep all the runtime code in the same style. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:188: for (i = 0; i < hdr->e_shnum; i++) { Write this as: for(i=0; i<hdr->e_shnum; i++) { http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:189: if (sh[i].sh_type == SHT_DYNSYM) { s/if (/if(/ Do this here and elsewhere throughout the file. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:195: * We need two things from the segment table: the load offset Comments within functions are normally written with //, here and elsewhere. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:198: for (i = 0; i < hdr->e_phnum; i++) { for(i=0; i < hdr->e_phnum; i++) { http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:209: if (!found_vaddr || !dyn) s/!dyn/dyn == nil/ http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:215: for (i = 0; dyn[i].d_tag != DT_NULL; i++) { for(i = 0; dyn[i].d_tag!=D_NULL; i++) { http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:216: switch (dyn[i].d_tag) { s/switch (/switch(/ http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:239: if (!vdso_info.symstrings || !vdso_info.symtab) use == nil, not ! http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:242: if (!vdso_info.verdef) use == nil http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:256: !runtime·strcmp(linux26.version, vdso_info.symstrings + aux->vda_name)) { use == 0, not ! http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:277: for (int32 i = 0; i < vdso_info.num_sym; i++) { Declare i at the top of the function, then for(i = 0; i<vdso_info.num_sym; i++) {
Sign in to reply to this message.
>Have you considered rsc's suggestion of delaying this code until later, to avoid the stack overflow problem? As far as I know nothing in the runtime calls these functions before setting up the stack, though it's certainly worth checking. I am within stack budget with this code, and it's checked by the compiler. So, there's no need to complicate the vdso setup. If, at some point, this code will be hard to modify and keep it within the stack budget, delaying the setup might be an answer. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:188: for (i = 0; i < hdr->e_shnum; i++) { On 2012/08/09 18:21:54, iant wrote: > Write this as: > for(i=0; i<hdr->e_shnum; i++) { Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:189: if (sh[i].sh_type == SHT_DYNSYM) { On 2012/08/09 18:21:54, iant wrote: > s/if (/if(/ > > Do this here and elsewhere throughout the file. Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:195: * We need two things from the segment table: the load offset On 2012/08/09 18:21:54, iant wrote: > Comments within functions are normally written with //, here and elsewhere. Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:198: for (i = 0; i < hdr->e_phnum; i++) { On 2012/08/09 18:21:54, iant wrote: > for(i=0; i < hdr->e_phnum; i++) { Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:209: if (!found_vaddr || !dyn) On 2012/08/09 18:21:54, iant wrote: > s/!dyn/dyn == nil/ Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:215: for (i = 0; dyn[i].d_tag != DT_NULL; i++) { On 2012/08/09 18:21:54, iant wrote: > for(i = 0; dyn[i].d_tag!=D_NULL; i++) { Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:216: switch (dyn[i].d_tag) { On 2012/08/09 18:21:54, iant wrote: > s/switch (/switch(/ Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:239: if (!vdso_info.symstrings || !vdso_info.symtab) On 2012/08/09 18:21:54, iant wrote: > use == nil, not ! Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:242: if (!vdso_info.verdef) On 2012/08/09 18:21:54, iant wrote: > use == nil Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:256: !runtime·strcmp(linux26.version, vdso_info.symstrings + aux->vda_name)) { On 2012/08/09 18:21:54, iant wrote: > use == 0, not ! Done. http://codereview.appspot.com/6454046/diff/1016/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:277: for (int32 i = 0; i < vdso_info.num_sym; i++) { On 2012/08/09 18:21:54, iant wrote: > Declare i at the top of the function, then > for(i = 0; i<vdso_info.num_sym; i++) { Done.
Sign in to reply to this message.
Can you please update the CL description and also the C code to note the Linux kernel version in which the vdso appeared? Maybe where I've requested the fallbacks you can put something like // Initialize to vsyscall function pointers, for use when vdso is unavailable // (kernel version < 2.999). Thanks. http://codereview.appspot.com/6454046/diff/3026/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/3026/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:163: void* runtime·__vdso_time_sym; Can you initialize these to the fallback values and then not need the comparisons in the assembly code?
Sign in to reply to this message.
On 2012/08/09 20:40:57, krasin wrote: > >Have you considered rsc's suggestion of delaying this code until later, to > avoid the stack overflow problem? As far as I know nothing in the runtime calls > these functions before setting up the stack, though it's certainly worth > checking. > > I am within stack budget with this code, and it's checked by the compiler. So, > there's no need to complicate the vdso setup. One advantage of delaying would be that many of the variables could move from being static variables to the stack.
Sign in to reply to this message.
I have removed checks from the syscalls, and initialized runtime·__vdso_time_sym and runtime·__vdso_gettimeofday_sym vars with the fallback values. vdso setup postponed and all but one pragmas are removed. Almost all global variables were eliminated. Please, take another look. http://codereview.appspot.com/6454046/diff/3026/src/pkg/runtime/vdso_linux_am... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/3026/src/pkg/runtime/vdso_linux_am... src/pkg/runtime/vdso_linux_amd64.c:163: void* runtime·__vdso_time_sym; On 2012/08/09 20:59:18, rsc wrote: > Can you initialize these to the fallback values and then not need the > comparisons in the assembly code? Done.
Sign in to reply to this message.
Looking very close. Have you signed the copyright license agreement mentioned at the bottom of http://golang.org/doc/contribute.html ? http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... src/pkg/runtime/vdso_linux_amd64.c:316: vdso_init_from_sysinfo_ehdr((Elf64_Ehdr*)elf_auxv[i].a_un.a_val); Will it work to make vdso_info a local variable of this function, and pass the address go vdso_init_from_sysinfo_ehdr and vdso_parse_symbols? http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... src/pkg/runtime/vdso_linux_amd64.c:325: static void linux_sysargs(int32 argc, uint8** argv) { What is this intermediate function for?
Sign in to reply to this message.
On 2012/08/10 23:05:06, iant wrote: > Looking very close. > > Have you signed the copyright license agreement mentioned at the bottom of > http://golang.org/doc/contribute.html ? Ian, I am sorry for some sort of confusion, because I post comments from different emails: krasin@google.com / krasin@golang.org / imkrasin@gmail.com At the same time, since my code is based on https://github.com/torvalds/linux/blob/v3.5/Documentation/vDSO/parse_vdso.c#L2 written by Andrew Lutomirski, we might want to ask him to sign the agreement or (alternatively) I can remove the rest of the code from parse_vdso, because it was almost completely rewritten. > > http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd64.c > File src/pkg/runtime/vdso_linux_amd64.c (right): > > http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... > src/pkg/runtime/vdso_linux_amd64.c:316: > vdso_init_from_sysinfo_ehdr((Elf64_Ehdr*)elf_auxv[i].a_un.a_val); > Will it work to make vdso_info a local variable of this function, and pass the > address go vdso_init_from_sysinfo_ehdr and vdso_parse_symbols? > > http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... > src/pkg/runtime/vdso_linux_amd64.c:325: static void linux_sysargs(int32 argc, > uint8** argv) { > What is this intermediate function for?
Sign in to reply to this message.
http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... src/pkg/runtime/vdso_linux_amd64.c:316: vdso_init_from_sysinfo_ehdr((Elf64_Ehdr*)elf_auxv[i].a_un.a_val); On 2012/08/10 23:05:06, iant wrote: > Will it work to make vdso_info a local variable of this function, and pass the > address go vdso_init_from_sysinfo_ehdr and vdso_parse_symbols? Done. http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... src/pkg/runtime/vdso_linux_amd64.c:325: static void linux_sysargs(int32 argc, uint8** argv) { On 2012/08/10 23:05:06, iant wrote: > What is this intermediate function for? I just was not fully sure that runtime·linux_setup_sysargs_handler and linux_sysargs belong to this file, because they are not vdso-related and might be used in other contexts as well. runtime_linux_amd64.c might be a better option. Anyway, I have removed linux_sysargs from the patch.
Sign in to reply to this message.
On 2012/08/11 00:01:16, krasin wrote: > On 2012/08/10 23:05:06, iant wrote: > > Looking very close. > > > > Have you signed the copyright license agreement mentioned at the bottom of > > http://golang.org/doc/contribute.html ? > Ian, > > I am sorry for some sort of confusion, because I post comments from different > emails: mailto:krasin@google.com / mailto:krasin@golang.org / mailto:imkrasin@gmail.com > > At the same time, since my code is based on > https://github.com/torvalds/linux/blob/v3.5/Documentation/vDSO/parse_vdso.c#L2 > written by Andrew Lutomirski, we might want to ask him to sign the agreement In case, if it was not spotted, the Go issue was opened by Andrew: https://code.google.com/p/go/issues/detail?id=1933 so he is aware of our effort. > (alternatively) I can remove the rest of the code from parse_vdso, because it > was almost completely rewritten. > > > > > > http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd64.c > > File src/pkg/runtime/vdso_linux_amd64.c (right): > > > > > http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... > > src/pkg/runtime/vdso_linux_amd64.c:316: > > vdso_init_from_sysinfo_ehdr((Elf64_Ehdr*)elf_auxv[i].a_un.a_val); > > Will it work to make vdso_info a local variable of this function, and pass the > > address go vdso_init_from_sysinfo_ehdr and vdso_parse_symbols? > > > > > http://codereview.appspot.com/6454046/diff/23/src/pkg/runtime/vdso_linux_amd6... > > src/pkg/runtime/vdso_linux_amd64.c:325: static void linux_sysargs(int32 argc, > > uint8** argv) { > > What is this intermediate function for?
Sign in to reply to this message.
On Fri, Aug 10, 2012 at 5:01 PM, <krasin@google.com> wrote: > On 2012/08/10 23:05:06, iant wrote: >> >> Looking very close. > > >> Have you signed the copyright license agreement mentioned at the > > bottom of >> >> http://golang.org/doc/contribute.html ? > > Ian, > > I am sorry for some sort of confusion, because I post comments from > different emails: krasin@google.com / krasin@golang.org / > imkrasin@gmail.com Whoops, sorry about that, fooled by the 'm'. Ian
Sign in to reply to this message.
What's the status of this CL? Is any action expected from me?
Sign in to reply to this message.
On 2012/08/13 18:01:55, imkrasin wrote: > What's the status of this CL? Is any action expected from me? friendly ping
Sign in to reply to this message.
Sorry for letting this slide. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.c File src/pkg/runtime/runtime.c (right): http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.c#ne... src/pkg/runtime/runtime.c:69: if(sysargs != nil) This should be named runtime·sysargs. Also it should be actually defined somewhere--that is, add void (*runtime·sysargs)(int32, uint8**); above this function. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.h#ne... src/pkg/runtime/runtime.h:808: void (*sysargs)(int32, uint8**); This should be declared extern, and it should be moved to the section under the comment "external data". Also, s/sysargs/runtime·sysargs/ http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:264: if(vdso_info->valid == false) Blank line after variable declarations. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:326: sysargs = runtime·linux_setup_vdso; s/sysargs/runtime·sysargs/
Sign in to reply to this message.
Please, take another look. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.c File src/pkg/runtime/runtime.c (right): http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.c#ne... src/pkg/runtime/runtime.c:69: if(sysargs != nil) On 2012/08/15 20:51:02, iant wrote: > This should be named runtime·sysargs. Also it should be actually defined > somewhere--that is, add > > void (*runtime·sysargs)(int32, uint8**); > > above this function. Done. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/runtime.h#ne... src/pkg/runtime/runtime.h:808: void (*sysargs)(int32, uint8**); On 2012/08/15 20:51:02, iant wrote: > This should be declared extern, and it should be moved to the section under the > comment "external data". Also, > s/sysargs/runtime·sysargs/ Done. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:264: if(vdso_info->valid == false) On 2012/08/15 20:51:02, iant wrote: > Blank line after variable declarations. Done. http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:326: sysargs = runtime·linux_setup_vdso; On 2012/08/15 20:51:02, iant wrote: > s/sysargs/runtime·sysargs/ Done.
Sign in to reply to this message.
LGTM Thanks for the patience and hard work. I would like rsc to sign off on this when he has time.
Sign in to reply to this message.
Thank you for the review, Ian.
Sign in to reply to this message.
On 2012/08/15 23:16:35, krasin wrote: > Thank you for the review, Ian. Russ, any update on this?
Sign in to reply to this message.
daily ping
Sign in to reply to this message.
On 2012/08/22 23:42:58, imkrasin wrote: > daily ping honestly, I am going to abandon this CL in a week.
Sign in to reply to this message.
We are waiting for rsc to sign off. He has been away but is back and has a huge backlog. Please be patient. -rob
Sign in to reply to this message.
On 2012/08/29 05:29:25, r wrote: > We are waiting for rsc to sign off. He has been away but is back and > has a huge backlog. Please be patient. > > -rob Thanks, I will wait.
Sign in to reply to this message.
This turned out nicely self-contained. Thanks very much. One small thing below to avoid touching the startup code. http://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/rt0_linux_am... File src/pkg/runtime/rt0_linux_amd64.s (right): http://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/rt0_linux_am... src/pkg/runtime/rt0_linux_amd64.s:8: CALL runtime·linux_setup_sysargs_handler(SB) Delete. http://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/vdso_linux_a... File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/vdso_linux_a... src/pkg/runtime/vdso_linux_amd64.c:323: #pragma textflag 7 Replace this function wth void (*runtime·sysargs)(int32, uint8**) = runtime.linux_setup_vdso; then the linker will do it for you.
Sign in to reply to this message.
https://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/rt0_linux_a... File src/pkg/runtime/rt0_linux_amd64.s (right): https://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/rt0_linux_a... src/pkg/runtime/rt0_linux_amd64.s:8: CALL runtime·linux_setup_sysargs_handler(SB) On 2012/08/31 17:28:55, rsc wrote: > Delete. Done. https://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/vdso_linux_... File src/pkg/runtime/vdso_linux_amd64.c (right): https://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/vdso_linux_... src/pkg/runtime/vdso_linux_amd64.c:323: #pragma textflag 7 On 2012/08/31 17:28:55, rsc wrote: > Replace this function wth > > void (*runtime·sysargs)(int32, uint8**) = runtime.linux_setup_vdso; > > then the linker will do it for you. Done.
Sign in to reply to this message.
Fixes issue 1933. in the description?
Sign in to reply to this message.
On 2012/08/31 21:37:46, albert.strasheim wrote: > Fixes issue 1933. > > in the description? Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=56ea40aac72b *** runtime: add vdso support for linux/amd64. Fixes issue 1933. R=iant, imkrasin, krasin, iant, minux.ma, rsc, nigeltao, r, fullung CC=golang-dev http://codereview.appspot.com/6454046 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
On 2012/08/31 22:06:37, rsc wrote: > LGTM There's a not-so-small change that this CL will break some non-x86-64 buildbot, but I expect that it would be something trivial.
Sign in to reply to this message.
On 2012/08/31 22:07:49, imkrasin wrote: > On 2012/08/31 22:06:37, rsc wrote: > > LGTM > > There's a not-so-small change that this CL will break some non-x86-64 buildbot, > but I expect that it would be something trivial. s/change/chance
Sign in to reply to this message.
|