[MIPS] Untrusted versions of setjmp and longjmp.

[MIPS] Untrusted versions of setjmp and longjmp.

Implementation of setjmp/longjmp.

The commit queue is not working for NaCl at the moment, so we are
committing with:
NOTRY=true
However, we have run a try job manually, and it passed fine

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2275 
TEST= None

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=9946

[Mark Seaborn, 2012-10-03 00:46:33]

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S
File pnacl/support/setjmp_mips32.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:1: /*
Surely this file should be "setjmp_mips.S"?  The other MIPS files don't have
"mips32" in their names.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:13: .ent setjmp
BTW, you didn't use .ent in tls_get_addr.S

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:25: 
It's a little odd that the empty lines group this by pairs of registers.

I'd suggest either putting an empty line between each reg:

and  $a0, $a0, STORE_MASK
sw   $s0, 0($a0)

and  $a0, $a0, STORE_MASK
sw   $s1, 0($a0)

and  $a0, $a0, STORE_MASK
sw   $s2, 0($a0)

or group by type of register, e.g.

/* Save s0-s7 */
and  $a0, $a0, STORE_MASK
sw   $s0, 0($a0)
and  $a0, $a0, STORE_MASK
sw   $s1, 0($a0)
and  $a0, $a0, STORE_MASK
sw   $s2, 0($a0)

/* Save sp */
...

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S
File pnacl/support/tls_get_addr.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:17: __tls_get_addr:
This function returns the thread's user thread pointer, right?  That's not what
__tls_get_addr() normally does.  __tls_get_addr(void *x) is normally defined by
ld.so; it takes a GOT slot entry address and returns the address of a TLS
variable.

NaCl currently has __nacl_read_tp() which used to be wrongly named
__tls_get_addr(), but that naming conflicted with dynamic linking so we fixed
it.

You could rename this to __nacl_read_tp(), but it might conflict with our
portable implementation in src/untrusted/nacl/nacl_read_tp.c.  More likely, it
just won't get used.  What is leading you to define this function now?

Since defining this in assembly is just an optimisation, you might consider
omitting this definition for now.  (And it's not an ideal optimisation if you
have to call a function to read the thread pointer.)

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:19: addiu   $v0, $v0, 0x7000
What is 0x7000 for?

[petarj, 2012-10-03 16:50:00]

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S
File pnacl/support/setjmp_mips32.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:1: /*
On 2012/10/03 00:46:33, Mark Seaborn wrote:
> Surely this file should be "setjmp_mips.S"?  The other MIPS files don't have
> "mips32" in their names.

Well, the other mips files are not compiled with "setjmp_${arch/-/_}".
See pnacl/build.sh

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:25: 
On 2012/10/03 00:46:33, Mark Seaborn wrote:
> It's a little odd that the empty lines group this by pairs of registers.
> 
> I'd suggest either putting an empty line between each reg:
> 
> and  $a0, $a0, STORE_MASK
> sw   $s0, 0($a0)
> 
> and  $a0, $a0, STORE_MASK
> sw   $s1, 0($a0)
> 
> and  $a0, $a0, STORE_MASK
> sw   $s2, 0($a0)
> 
> or group by type of register, e.g.
> 
> /* Save s0-s7 */
> and  $a0, $a0, STORE_MASK
> sw   $s0, 0($a0)
> and  $a0, $a0, STORE_MASK
> sw   $s1, 0($a0)
> and  $a0, $a0, STORE_MASK
> sw   $s2, 0($a0)
> 
> /* Save sp */
> ...

Normally, we would group it by type of registers (GPR and FPR), like it is done
in newlib. But the idea was to show the bundle alignment, so it is more readable
for anyone looking for pattern breakage. This way, for instance, an extra 'nop'
can be explained without words.
It was not grouped by pairs of registers.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S
File pnacl/support/tls_get_addr.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:17: __tls_get_addr:
On 2012/10/03 00:46:33, Mark Seaborn wrote:
> This function returns the thread's user thread pointer, right? 

Correct.

> That's not what
> __tls_get_addr() normally does.  __tls_get_addr(void *x) is normally defined
by
> ld.so; it takes a GOT slot entry address and returns the address of a TLS
> variable.
> 
> NaCl currently has __nacl_read_tp() which used to be wrongly named
> __tls_get_addr(), but that naming conflicted with dynamic linking so we fixed
> it.

At the time we implemented it, your version was also called __tls_get_addr.
We tend to be consistent with the rest of the code even in errors. :)

I now see that Jan has changed that.
It should be renamed, true.
 
> You could rename this to __nacl_read_tp(), but it might conflict with our
> portable implementation in src/untrusted/nacl/nacl_read_tp.c.  More likely, it
> just won't get used.  What is leading you to define this function now?

We changed LLVM to generate a call to it, in the same vain ARM calls
__aeabi_read_tp. So we need it like ARM needs aeabi_read_tp.S in the same
directory. Do you disagree?

> 
> Since defining this in assembly is just an optimisation, you might consider
> omitting this definition for now.  (And it's not an ideal optimisation if you
> have to call a function to read the thread pointer.)

It seems cheaper than to go through the syscall routine.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:19: addiu   $v0, $v0, 0x7000
On 2012/10/03 00:46:33, Mark Seaborn wrote:
> What is 0x7000 for?

$t8 points to the start of the TLS data area. The thread pointer is offset by
0x7000. This way more data is accessible with signed 16-bit offsets from tp.
This is according to TLS/NPTL ABI for MIPS.

[Mark Seaborn, 2012-10-03 20:02:54]

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S
File pnacl/support/setjmp_mips32.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:1: /*
On 2012/10/03 16:50:01, petarj wrote:
> On 2012/10/03 00:46:33, Mark Seaborn wrote:
> > Surely this file should be "setjmp_mips.S"?  The other MIPS files don't have
> > "mips32" in their names.
> 
> Well, the other mips files are not compiled with "setjmp_${arch/-/_}".
> See pnacl/build.sh

Hmm.  Given that build.sh is already munging '-' to '_', there is a case for
further munging "mips32" to "mips".  But I guess it's OK as is.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:25: 
On 2012/10/03 16:50:01, petarj wrote:
> Normally, we would group it by type of registers (GPR and FPR), like it
> is done in newlib. But the idea was to show the bundle alignment,

Ah, I see.  Maybe add a comment like
/* The code here is grouped by instruction bundle */

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S
File pnacl/support/tls_get_addr.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:17: __tls_get_addr:
On 2012/10/03 16:50:01, petarj wrote:
> On 2012/10/03 00:46:33, Mark Seaborn wrote:
> > This function returns the thread's user thread pointer, right? 
> 
> Correct.
> 
> > That's not what
> > __tls_get_addr() normally does.  __tls_get_addr(void *x) is normally
> > defined by ld.so; it takes a GOT slot entry address and returns the address
> > of a TLS variable.
> > 
> > NaCl currently has __nacl_read_tp() which used to be wrongly named
> > __tls_get_addr(), but that naming conflicted with dynamic linking so we
> > fixed it.
> 
> At the time we implemented it, your version was also called __tls_get_addr.

Wow, that's a long time ago!  We renamed the wrong __tls_get_addr() in 2010, I
think.

> We tend to be consistent with the rest of the code even in errors. :)

Yes, I've noticed. :-)  You know, if our ARM or x86 code does stupid things, it
would be better if you sent us fixes rather than copying the stupid things. :-)

> I now see that Jan has changed that.
> It should be renamed, true.
>  
> > You could rename this to __nacl_read_tp(), but it might conflict with our
> > portable implementation in src/untrusted/nacl/nacl_read_tp.c.  More likely,
it
> > just won't get used.  What is leading you to define this function now?
> 
> We changed LLVM to generate a call to it, in the same vain ARM calls
> __aeabi_read_tp. So we need it like ARM needs aeabi_read_tp.S in the same
> directory. Do you disagree?

First let me give some background.  The current implementation of TLS in PNaCl
does some dubious things.  My current plan is to change PNaCl TLS as described
here:  https://code.google.com/p/nativeclient/issues/detail?id=2837

In PNaCl currently:

 * ARM: The translator is *supposed* to convert TLS variables to r9 accesses. 
This means it *shouldn't* normally generate calls to __aeabi_read_tp(). 
However, we build the IRT with -mtls-use-call so that __aeabi_read_tp() calls
are generated, but for the IRT we replace __aeabi_read_tp() with an
implementation that returns the IRT's thread pointer.

 * x86-64: The translator generates calls to __nacl_read_tp().

There is a discrepancy between ARM and x86-64 in PNaCl:

ARM's __aeabi_read_tp() is linked in by the translator (system code), but
x86-64's __nacl_read_tp() is expected to be provided by user code.

The fact that system-generated code depends on a symbol provided by user code is
a bug which we need to fix.  However, for the time being, MIPS could rely on
__nacl_read_tp() being provided by user code as x86-64 does.


> > Since defining this in assembly is just an optimisation, you might consider
> > omitting this definition for now.  (And it's not an ideal optimisation if
you
> > have to call a function to read the thread pointer.)
> 
> It seems cheaper than to go through the syscall routine.

But cheaper still is for the translator to generate code that reads $t8 inline,
rather than generating a function call.

You should also bear in mind that we are planning on changing how we use r9 for
TLS on ARM:
https://code.google.com/p/nativeclient/issues/detail?id=2933

You don't need to do the same for MIPS, but it might make things easier if you
do.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:19: addiu   $v0, $v0, 0x7000
On 2012/10/03 16:50:01, petarj wrote:
> On 2012/10/03 00:46:33, Mark Seaborn wrote:
> > What is 0x7000 for?
> 
> $t8 points to the start of the TLS data area. The thread pointer is offset
> by 0x7000. This way more data is accessible with signed 16-bit offsets from
> tp. This is according to TLS/NPTL ABI for MIPS.

Hmm, yes, I found some references to that by searching for "mips
__tls_get_addr".  Do you have a canonical reference for Linux's MIPS TLS ABI?  I
found some docs but it was not clear which was authoritative.

It's not clear to me how this offset of 0x7000 will interact with the thread
pointer munging that is done in src/untrusted/nacl/tls_params.h.

Currently tls_params.h in PNaCl is a huge hack.  User code is expected to know
about the different ARM and x86 TLS layouts.  There are PNaCl LLVM intrinsics to
tell user code which to use.  Currently, MIPS-PNaCl would have to pick either
the ARM-style or x86-style TLS layout -- and it's not clear how the 0x7000
offset fits in with either layout.

However, when I've implemented
https://code.google.com/p/nativeclient/issues/detail?id=2837, these layout hacks
should go away.


In conclusion:

Please read
https://code.google.com/p/nativeclient/issues/detail?id=2837 (PNaCl ABI) and
https://code.google.com/p/nativeclient/issues/detail?id=2933 (ARM ABI)
and let me know if you have any questions.  The two are largely independent,
though.

For now, I think you should be able to remove tls_get_addr.S.  If your MIPS
translator generates calls to __nacl_read_tp(), you can currently rely on user
code to define that symbol.  Does that work for you?  If not, I could give you
the benefit of the doubt and OK this change (with tls_get_addr.S renamed).

[Mark Seaborn, 2012-10-04 17:33:13]

On 3 October 2012 13:02, <mseaborn@chromium.org> wrote:

> There is a discrepancy between ARM and x86-64 in PNaCl:
>
> ARM's __aeabi_read_tp() is linked in by the translator (system code),
> but x86-64's __nacl_read_tp() is expected to be provided by user code.
>
> The fact that system-generated code depends on a symbol provided by user
> code is a bug which we need to fix.  However, for the time being, MIPS
> could rely on __nacl_read_tp() being provided by user code as x86-64
> does.
>

OK, I've filed a bug for that problem now:
https://code.google.com/p/nativeclient/issues/detail?id=3073

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[petarj, 2012-10-05 17:26:44]

Second patch set uploaded.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S
File pnacl/support/setjmp_mips32.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/setjmp_mips32.S#...
pnacl/support/setjmp_mips32.S:25: 
On 2012/10/03 20:02:54, Mark Seaborn wrote:
> On 2012/10/03 16:50:01, petarj wrote:
> > Normally, we would group it by type of registers (GPR and FPR), like it
> > is done in newlib. But the idea was to show the bundle alignment,
> 
> Ah, I see.  Maybe add a comment like
> /* The code here is grouped by instruction bundle */

Done.

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S
File pnacl/support/tls_get_addr.S (right):

http://codereview.chromium.org/11039026/diff/1/pnacl/support/tls_get_addr.S#n...
pnacl/support/tls_get_addr.S:19: addiu   $v0, $v0, 0x7000
On 2012/10/03 20:02:54, Mark Seaborn wrote:
> Hmm, yes, I found some references to that by searching for "mips
> __tls_get_addr".  Do you have a canonical reference for Linux's MIPS TLS ABI? 
I
> found some docs but it was not clear which was authoritative.

The best documentation I can point you to is:

http://www.linux-mips.org/wiki/NPTL

> For now, I think you should be able to remove tls_get_addr.S.  If your MIPS
> translator generates calls to __nacl_read_tp(), you can currently rely on user
> code to define that symbol.  Does that work for you?

It should. I have removed the tls_get_addr.S from the patch.

[Mark Seaborn, 2012-10-05 18:06:53]

LGTM

[commit-bot: I haz the power, 2012-10-05 21:50:43]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11039026/10004

[petarj, 2012-10-06 00:30:26]

All tries on trybots have successeded when manually run.

http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-win7-pnacl...
http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-linux-pnac...
http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-mac-pnacl-...
http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-linux-pnac...

[Mark Seaborn, 2012-10-06 00:42:12]

On 5 October 2012 17:30, <petarj@mips.com> wrote:

> All tries on trybots have successeded when manually run.
>

Feel free to commit this with NOTRY=true.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[commit-bot: I haz the power, 2012-10-06 00:44:03]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11039026/10004

[petarj, 2012-10-06 01:14:33]

On 2012/10/06 00:44:03, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://nativeclient-status.appspot.com/cq/petarj%40mips.com/11039026/10004

It seems that NOTRY=true does not work for me in pnacl dir.

Regards,
Petar

[commit-bot: I haz the power, 2012-10-08 13:51:51]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-10-08 14:08:56]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11039026/10004

[commit-bot: I haz the power, 2012-10-08 14:09:03]

Change committed as 9946