Re: runtime: add vdso support for linux/amd64. (issue 6454046)

[imkr...@gmail.com]

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_amd64.c
File src/pkg/runtime/vdso_linux_amd64.c (right):

http://codereview.appspot.com/6454046/diff/3026/src/pkg/runtime/vdso_linux_amd64.c#newcode163
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.

http://codereview.appspot.com/6454046/

[ia...@golang.org]

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_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);
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_amd64.c#newcode325
src/pkg/runtime/vdso_linux_amd64.c:325: static void linux_sysargs(int32
argc, uint8** argv) {
What is this intermediate function for?

http://codereview.appspot.com/6454046/

[kra...@google.com]

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: kra...@google.com / kra...@golang.org /
imkr...@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.

[kra...@google.com]

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 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_amd64.c#newcode325
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.

http://codereview.appspot.com/6454046/

[kra...@google.com]

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:kra...@google.com / mailto:kra...@golang.org /
mailto:imkr...@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_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);

> > 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_amd64.c#newcode325
> > src/pkg/runtime/vdso_linux_amd64.c:325: static void
linux_sysargs(int32 argc,
> > uint8** argv) {

> > What is this intermediate function for?

http://codereview.appspot.com/6454046/

[Ian Lance Taylor]

On Fri, Aug 10, 2012 at 5:01 PM, <kra...@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: kra...@google.com / kra...@golang.org /
> imkr...@gmail.com

Whoops, sorry about that, fooled by the 'm'.

Ian

[imkr...@gmail.com]

What's the status of this CL? Is any action expected from me?

http://codereview.appspot.com/6454046/

[imkr...@gmail.com]

On 2012/08/13 18:01:55, imkrasin wrote:
> What's the status of this CL? Is any action expected from me?

friendly ping

http://codereview.appspot.com/6454046/

[ia...@golang.org]

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 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#newcode808
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_amd64.c
File src/pkg/runtime/vdso_linux_amd64.c (right):

http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_amd64.c#newcode264
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_amd64.c#newcode326
src/pkg/runtime/vdso_linux_amd64.c:326: sysargs =
runtime·linux_setup_vdso;
s/sysargs/runtime·sysargs/

http://codereview.appspot.com/6454046/

[imkr...@gmail.com]

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 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#newcode808
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_amd64.c
File src/pkg/runtime/vdso_linux_amd64.c (right):

http://codereview.appspot.com/6454046/diff/19007/src/pkg/runtime/vdso_linux_amd64.c#newcode264
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_amd64.c#newcode326
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.

http://codereview.appspot.com/6454046/

[ia...@golang.org]

LGTM

Thanks for the patience and hard work.

I would like rsc to sign off on this when he has time.


http://codereview.appspot.com/6454046/

[kra...@google.com]

Thank you for the review, Ian.

http://codereview.appspot.com/6454046/