Put __pnacl_real_irt_interface variable in bss instead of common.

Put __pnacl_real_irt_interface variable in bss instead of common.

Then we don't have to handle SHN_COMMON in the native link.
Normally, pnacl-clang adds -fno-common to force definition
of common symbols. However, the x86 shims are compiled by
nacl-gcc so it showed up.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=3762
R=mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243473

[Mark Seaborn, 2014-01-07 22:22:28]

This is curious, because I didn't realise there was a difference between
  int x = 0;
and
  int x;

With Linux clang there is.  With pnacl-clang there isn't.  That's odd.  Does
Clang's le32 have a setting saying "common" doesn't work, perhaps?

Are we compiling this file with gcc, or are we using pnacl-clang with a
--pnacl-frontend-triple that enables "common"?

LGTM I suppose, but this seems rather subtle.  Have you considered other ways of
doing this?  e.g. Stripping out "common" in IR?  How do you intend to implement
Subzero's linking?

[Mark Seaborn, 2014-01-07 22:22:59]

https://codereview.chromium.org/116043012/diff/1/ppapi/native_client/src/untr...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/shim_ppapi.c (right):

https://codereview.chromium.org/116043012/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_irt_shim/shim_ppapi.c:93:
TYPE_nacl_irt_query __pnacl_real_irt_interface = 0;
BTW, this should be NULL rather than 0.

[jvoung (off chromium), 2014-01-07 22:58:21]

On 2014/01/07 22:22:28, Mark Seaborn wrote:
> This is curious, because I didn't realise there was a difference between
>   int x = 0;
> and
>   int x;
> 
> With Linux clang there is.  With pnacl-clang there isn't.  That's odd.  Does
> Clang's le32 have a setting saying "common" doesn't work, perhaps?

The pnacl-driver.py adds the clang flag "-fno-common", so usually we don't worry
about this (it defines them to 0).


> Are we compiling this file with gcc, or are we using pnacl-clang with a
> --pnacl-frontend-triple that enables "common"?

On x86 these shim files are currently compiled with nacl-gcc instead of the
--pnacl-frontend-triple, so that is why we still see common. On ARM it does
happen to use pnacl-clang w/ the --pnacl-frontend-triple though.


> LGTM I suppose, but this seems rather subtle.  Have you considered other ways
of
> doing this?  e.g. Stripping out "common" in IR?  How do you intend to
implement
> Subzero's linking?

Yeah it is subtle but usually we won't run into this because of -fno-common.  We
saw a difference with the way gold handled common vs the bfd linker, for some
situation with unspecified behavior, but I don't remember the details. After
that we added the -fno-common to clang's invocation.

Haven't planned it out too much, but I have been prototyping simple static ELF
linker in a higher-level language (so not really reusable as is...), just to see
what's involved if we keep ELF. So far, Jim and Eli have advocated for keeping
ELF as the container format instead of something simpler so that LLVM wouldn't
need to change too much, and we could reuse objdump/readelf, etc.

Maybe we could simplify things a bit more and avoid handling of archives, by
linking together the various archives to a single .o file w/ -r, but that would
have to happen after we build the IRT shim on the chrome side.

It seems good to be able to do the linking in the same process as the
translators to cut out the process creation cost. The more aggressive thing is
to be able to subzero-compile, link, and run in the same process, but we'd need
something to clear out the subzero/linker state...

It also seems like the linker will need to understand a bit about extra sections
that might be needed for the trampoline idea.

*) The O0/subzero linker would output an executable, but it would keep a
.rel.data.trampoline section with the addresses of the function pointers that
may need patching against O2 (even though it's been initially relocated to point
to the O0 functions).

*) The O2 translator would also output an executable, but it would be given
access to the O0/subzero nexe to have access to the symbol table for data
elements, and it would need some special treatment for that.

*) We'd need to reserve different start addresses for the O2
code/rodata.switch_tables, etc.

*) The O2 executable would keep the symbol table. Then something small (not the
full linker) would use the O2 symbol table and the O0 .rel.data.trampolines to
update the function pointers.

Some notes added to the end of Jim's
https://docs.google.com/a/google.com/document/d/1I93BeV1Rio7RbH2MqWAi7jzmm7Go...
document.

[jvoung (off chromium), 2014-01-07 23:58:59]

https://codereview.chromium.org/116043012/diff/1/ppapi/native_client/src/untr...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/shim_ppapi.c (right):

https://codereview.chromium.org/116043012/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_irt_shim/shim_ppapi.c:93:
TYPE_nacl_irt_query __pnacl_real_irt_interface = 0;
On 2014/01/07 22:22:59, Mark Seaborn wrote:
> BTW, this should be NULL rather than 0.

Done.

[commit-bot: I haz the power, 2014-01-08 00:02:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/116043012/90001

[commit-bot: I haz the power, 2014-01-08 02:23:46]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jvoung (off chromium), 2014-01-08 02:41:56]

Committed patchset #2 manually as r243473 (presubmit successful).

[Mark Seaborn, 2014-01-10 00:44:28]

On 7 January 2014 14:58, <jvoung@chromium.org> wrote:

> On 2014/01/07 22:22:28, Mark Seaborn wrote:
>
>> This is curious, because I didn't realise there was a difference between
>>    int x = 0;
>> and
>>    int x;
>>
>
>  With Linux clang there is.  With pnacl-clang there isn't.  That's odd.
>>  Does
>> Clang's le32 have a setting saying "common" doesn't work, perhaps?
>>
>
> The pnacl-driver.py adds the clang flag "-fno-common", so usually we don't
> worry
> about this (it defines them to 0).


Ah, that's interesting.  Presumably if we find any more PNaCl system code
for which the compiler generates a common symbol, we should compile the
system code with -fno-common rather than changing the source?


LGTM I suppose, but this seems rather subtle.  Have you considered other
>> ways of
>
>  doing this?  e.g. Stripping out "common" in IR?  How do you intend to
>> implement
>
>  Subzero's linking?
>>
>
> Yeah it is subtle but usually we won't run into this because of
> -fno-common.  We
> saw a difference with the way gold handled common vs the bfd linker, for
> some
> situation with unspecified behavior, but I don't remember the details.
> After
> that we added the -fno-common to clang's invocation.
>

Interesting.  -fno-common was added here:
https://src.chromium.org/viewvc/native_client?revision=8410&view=revision
https://src.chromium.org/viewvc/native_client?revision=8420&view=revision
for https://code.google.com/p/nativeclient/issues/detail?id=2707
("pnacl: switch to gold as the final linker" -- presumably referring to
native nexe linking rather than bitcode linking)

I wonder if this workaround is still needed?  Presumably bugs in Gold have
been fixed by now.

It's interesting that this doesn't seem to have broken any programs that
are being built with PNaCl.  Maybe we'll come across a program that relies
on common symbols and we'll want to fix this more cleanly.



> Haven't planned it out too much, but I have been prototyping simple static
> ELF
> linker in a higher-level language (so not really reusable as is...), just
> to see
> what's involved if we keep ELF. So far, Jim and Eli have advocated for
> keeping
> ELF as the container format instead of something simpler so that LLVM
> wouldn't
> need to change too much, and we could reuse objdump/readelf, etc.
>

I'd imagined you might just copy the PNaCl system code into the PNaCl
translator.  i.e. Have a script to convert an ELF .o file to C code like
this, which you would compile into the translator:

uint8_t code_section[] = "...";
uint8_t data_section[] = "...";
RelocInfo relocs[] = { ... };

Then Subzero doesn't need any C++ plumbing code for reading the .o file
from a filesystem or for parsing its ELF headers.



> Maybe we could simplify things a bit more and avoid handling of archives,
> by
> linking together the various archives to a single .o file w/ -r, but that
> would
> have to happen after we build the IRT shim on the chrome side.
>

Yes, I'd say you definitely want to avoid handling archives, and you can
link the system code with "-r" as you describe.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.