|
|
Created:
8 years, 2 months ago by petarj Modified:
8 years, 2 months ago Reviewers:
Mark Seaborn CC:
native-client-reviews_googlegroups.com Base URL:
http://src.chromium.org/native_client/trunk/src/native_client/ Visibility:
Public. |
Description[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
Patch Set 1 #
Total comments: 15
Patch Set 2 : Removing __tls_get_addr from the patch. #Patch Set 3 : Rebase. #
Messages
Total messages: 15 (0 generated)
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?
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.
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).
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.
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.
LGTM
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11039026/10004
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...
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.
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11039026/10004
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 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.
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11039026/10004
Change committed as 9946 |