Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2039)

Issue 6454046: runtime: add vdso support for linux/amd64.

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 8 months ago by Ivan Krasin
Modified:
10 years, 8 months ago
Reviewers:
imkrasin, rsc, iant
CC:
iant, krasin, iant2, minux1, rsc, nigeltao, r, albert.strasheim, golang-dev
Visibility:
Public.

Description

runtime: 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+330 lines, -2 lines) Patch
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/runtime.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +4 lines, -0 lines 0 comments Download
A src/pkg/runtime/vdso_linux_amd64.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 1 chunk +323 lines, -0 lines 0 comments Download
M src/pkg/syscall/asm_linux_amd64.s View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 56
iant
http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c#newcode5 src/pkg/runtime/vdso_linux_amd64.c:5: * This code is meant to be linked in ...
11 years, 8 months ago (2012-07-26 16:48:32 UTC) #1
imkrasin
http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c#newcode290 src/pkg/runtime/vdso_linux_amd64.c:290: /* That's all we need. */ On 2012/07/26 16:48:33, ...
11 years, 8 months ago (2012-07-26 17:22:50 UTC) #2
iant
http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c#newcode290 src/pkg/runtime/vdso_linux_amd64.c:290: /* That's all we need. */ > Do I ...
11 years, 8 months ago (2012-07-26 17:32:49 UTC) #3
krasin
On 2012/07/26 17:32:49, iant wrote: > http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c > File src/pkg/runtime/vdso_linux_amd64.c (right): > > http://codereview.appspot.com/6454046/diff/2001/src/pkg/runtime/vdso_linux_amd64.c#newcode290 > ...
11 years, 8 months ago (2012-07-26 18:09:46 UTC) #4
krasin
also, I have actually seen the buffer underflow that caused a segfault.
11 years, 8 months ago (2012-07-26 18:12:10 UTC) #5
iant2
Sorry, this is not something I know about.
11 years, 8 months ago (2012-07-26 18:18:33 UTC) #6
imkrasin
On 2012/07/26 18:18:33, iant2 wrote: > Sorry, this is not something I know about. np, ...
11 years, 8 months ago (2012-07-26 18:19:51 UTC) #7
minux1
On 2012/07/26 18:09:46, krasin wrote: > I do set #pragma texflag 7, and I see ...
11 years, 8 months ago (2012-07-27 00:34:39 UTC) #8
imkrasin
On 2012/07/27 00:34:39, minux wrote: > On 2012/07/26 18:09:46, krasin wrote: > > I do ...
11 years, 8 months ago (2012-07-27 00:39:15 UTC) #9
imkrasin
Hi Ian, please, take a first high-level look. Currently, this CL enables vdso-aware syscall.Time with ...
11 years, 8 months ago (2012-07-27 06:33:19 UTC) #10
iant
Some comments. I wouldn't have code in runtime #include code in cmd/ld. They are compiled ...
11 years, 8 months ago (2012-07-27 17:24:23 UTC) #11
imkrasin
Ian, please, take another look. http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/rt0_linux_amd64.s File src/pkg/runtime/rt0_linux_amd64.s (right): http://codereview.appspot.com/6454046/diff/14001/src/pkg/runtime/rt0_linux_amd64.s#newcode9 src/pkg/runtime/rt0_linux_amd64.s:9: CALL runtime·linux_find_vdso_symbols(SB) On 2012/07/27 ...
11 years, 8 months ago (2012-07-27 23:36:50 UTC) #12
iant
http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c#newcode131 src/pkg/runtime/vdso_linux_amd64.c:131: static struct vdso_info I don't think this struct needs ...
11 years, 8 months ago (2012-07-30 17:32:49 UTC) #13
krasin
http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c#newcode131 src/pkg/runtime/vdso_linux_amd64.c:131: static struct vdso_info On 2012/07/30 17:32:49, iant wrote: > ...
11 years, 8 months ago (2012-07-30 18:28:37 UTC) #14
iant
http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c#newcode347 src/pkg/runtime/vdso_linux_amd64.c:347: cur_sym = &sym_keys[VDSO_TIME_IND]; On 2012/07/30 18:28:37, krasin wrote: > ...
11 years, 8 months ago (2012-07-30 19:49:21 UTC) #15
rsc
I don't know if you're still having trouble with the textflag 7 overflows, but if ...
11 years, 7 months ago (2012-08-03 19:44:30 UTC) #16
rsc
(Then the handler doesn't have to be textflag 7.)
11 years, 7 months ago (2012-08-03 19:44:45 UTC) #17
krasin
On 2012/08/03 19:44:45, rsc wrote: > (Then the handler doesn't have to be textflag 7.) ...
11 years, 7 months ago (2012-08-03 19:48:01 UTC) #18
nigeltao
http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/5013/src/pkg/runtime/vdso_linux_amd64.c#newcode111 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_amd64.c#newcode199 src/pkg/runtime/vdso_linux_amd64.c:199: ...
11 years, 7 months ago (2012-08-06 01:08:59 UTC) #19
imkrasin
Please, take another look. http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/14008/src/pkg/runtime/vdso_linux_amd64.c#newcode347 src/pkg/runtime/vdso_linux_amd64.c:347: cur_sym = &sym_keys[VDSO_TIME_IND]; On 2012/07/30 ...
11 years, 7 months ago (2012-08-08 21:46:13 UTC) #20
iant
Thanks for working on this. Some more comments. http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_amd64.c#newcode165 src/pkg/runtime/vdso_linux_amd64.c:165: { ...
11 years, 7 months ago (2012-08-08 22:10:50 UTC) #21
imkrasin
PTAL http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12003/src/pkg/runtime/vdso_linux_amd64.c#newcode165 src/pkg/runtime/vdso_linux_amd64.c:165: { (byte*)"__vdso_time", 0xa33c485 }, On 2012/08/08 22:10:50, iant ...
11 years, 7 months ago (2012-08-09 00:43:27 UTC) #22
iant
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, ...
11 years, 7 months ago (2012-08-09 00:54:26 UTC) #23
iant
http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_amd64.c#newcode329 src/pkg/runtime/vdso_linux_amd64.c:329: if (ver > MAX_VER_IND || found_ver_ind[ver] != ver_ind_marker) We ...
11 years, 7 months ago (2012-08-09 00:56:59 UTC) #24
imkrasin
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: ...
11 years, 7 months ago (2012-08-09 01:53:40 UTC) #25
imkrasin
http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_amd64.c File src/pkg/runtime/vdso_linux_amd64.c (right): http://codereview.appspot.com/6454046/diff/12005/src/pkg/runtime/vdso_linux_amd64.c#newcode329 src/pkg/runtime/vdso_linux_amd64.c:329: if (ver > MAX_VER_IND || found_ver_ind[ver] != ver_ind_marker) On ...
11 years, 7 months ago (2012-08-09 01:53:50 UTC) #26
iant
OK, this is looking good. Thanks for your patience. Have you considered rsc's suggestion of ...
11 years, 7 months ago (2012-08-09 18:21:54 UTC) #27
krasin
>Have you considered rsc's suggestion of delaying this code until later, to avoid the stack ...
11 years, 7 months ago (2012-08-09 20:40:57 UTC) #28
rsc
Can you please update the CL description and also the C code to note the ...
11 years, 7 months ago (2012-08-09 20:59:17 UTC) #29
iant
On 2012/08/09 20:40:57, krasin wrote: > >Have you considered rsc's suggestion of delaying this code ...
11 years, 7 months ago (2012-08-09 21:50:18 UTC) #30
imkrasin
I have removed checks from the syscalls, and initialized runtime·__vdso_time_sym and runtime·__vdso_gettimeofday_sym vars with the ...
11 years, 7 months ago (2012-08-10 22:36:57 UTC) #31
iant
Looking very close. Have you signed the copyright license agreement mentioned at the bottom of ...
11 years, 7 months ago (2012-08-10 23:05:06 UTC) #32
krasin
On 2012/08/10 23:05:06, iant wrote: > Looking very close. > > Have you signed the ...
11 years, 7 months ago (2012-08-11 00:01:16 UTC) #33
krasin
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_amd64.c#newcode316 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 ...
11 years, 7 months ago (2012-08-11 00:01:38 UTC) #34
krasin
On 2012/08/11 00:01:16, krasin wrote: > On 2012/08/10 23:05:06, iant wrote: > > Looking very ...
11 years, 7 months ago (2012-08-11 00:05:59 UTC) #35
iant2
On Fri, Aug 10, 2012 at 5:01 PM, <krasin@google.com> wrote: > On 2012/08/10 23:05:06, iant ...
11 years, 7 months ago (2012-08-11 00:11:26 UTC) #36
imkrasin
What's the status of this CL? Is any action expected from me?
11 years, 7 months ago (2012-08-13 18:01:55 UTC) #37
imkrasin
On 2012/08/13 18:01:55, imkrasin wrote: > What's the status of this CL? Is any action ...
11 years, 7 months ago (2012-08-15 20:25:01 UTC) #38
iant
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#newcode69 src/pkg/runtime/runtime.c:69: if(sysargs != nil) This ...
11 years, 7 months ago (2012-08-15 20:51:02 UTC) #39
imkrasin
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#newcode69 src/pkg/runtime/runtime.c:69: if(sysargs != nil) On 2012/08/15 ...
11 years, 7 months ago (2012-08-15 22:55:29 UTC) #40
iant
LGTM Thanks for the patience and hard work. I would like rsc to sign off ...
11 years, 7 months ago (2012-08-15 23:14:50 UTC) #41
krasin
Thank you for the review, Ian.
11 years, 7 months ago (2012-08-15 23:16:35 UTC) #42
imkrasin
On 2012/08/15 23:16:35, krasin wrote: > Thank you for the review, Ian. Russ, any update ...
11 years, 7 months ago (2012-08-21 22:33:07 UTC) #43
imkrasin
daily ping
11 years, 7 months ago (2012-08-22 23:42:58 UTC) #44
imkrasin
On 2012/08/22 23:42:58, imkrasin wrote: > daily ping honestly, I am going to abandon this ...
11 years, 7 months ago (2012-08-29 04:27:13 UTC) #45
r
We are waiting for rsc to sign off. He has been away but is back ...
11 years, 7 months ago (2012-08-29 05:29:25 UTC) #46
imkrasin
On 2012/08/29 05:29:25, r wrote: > We are waiting for rsc to sign off. He ...
11 years, 7 months ago (2012-08-29 05:30:48 UTC) #47
rsc
This turned out nicely self-contained. Thanks very much. One small thing below to avoid touching ...
11 years, 7 months ago (2012-08-31 17:28:55 UTC) #48
imkrasin
https://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/rt0_linux_amd64.s File src/pkg/runtime/rt0_linux_amd64.s (right): https://codereview.appspot.com/6454046/diff/22001/src/pkg/runtime/rt0_linux_amd64.s#newcode8 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. ...
11 years, 7 months ago (2012-08-31 21:31:07 UTC) #49
albert.strasheim
Fixes issue 1933. in the description?
11 years, 7 months ago (2012-08-31 21:37:46 UTC) #50
imkrasin
On 2012/08/31 21:37:46, albert.strasheim wrote: > Fixes issue 1933. > > in the description? Done.
11 years, 7 months ago (2012-08-31 21:39:44 UTC) #51
rsc
LGTM
11 years, 7 months ago (2012-08-31 22:06:37 UTC) #52
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=56ea40aac72b *** runtime: add vdso support for linux/amd64. Fixes issue 1933. R=iant, ...
11 years, 7 months ago (2012-08-31 22:07:16 UTC) #53
imkrasin
On 2012/08/31 22:06:37, rsc wrote: > LGTM There's a not-so-small change that this CL will ...
11 years, 7 months ago (2012-08-31 22:07:49 UTC) #54
imkrasin
On 2012/08/31 22:07:49, imkrasin wrote: > On 2012/08/31 22:06:37, rsc wrote: > > LGTM > ...
11 years, 7 months ago (2012-08-31 22:08:07 UTC) #55
remyoudompheng
10 years, 8 months ago (2013-07-20 21:22:43 UTC) #56
R=close
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b