Issue 923253002: Use PNaCl's implementation of setjmp/longjmp for x86-64

Issue 923253002: Use PNaCl's implementation of setjmp/longjmp for x86-64 (Closed)

Created:
5 years, 10 months ago by Derek Schuff

Modified:
5 years, 10 months ago

Reviewers:
Mark Seaborn

CC:
native-client-reviews_googlegroups.com

Base URL:
https://chromium.googlesource.com/native_client/nacl-newlib.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Use PNaCl's implementation of setjmp/longjmp for x86-64 PNaCl's version does not write the sandbox base address into memory. The previous implementation also returned 0 instead of 1 when the 'val' argument was 0; PNaCl's version is correct. BUG= https://code.google.com/p/nativeclient/issues/detail?id=4088 R=mseaborn@chromium.org Committed: https://git.chromium.org/gitweb?p=native_client/nacl-newlib.git;a=commit;h=2cc581b64e949a00839e666ba2a36e069830aaf4

Patch Set 1 #

Total comments: 4

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+75 lines, -48 lines)			Patch
	M	newlib/libc/machine/x86_64/setjmp.S	View		1 chunk	+75 lines, -48 lines	4 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Mark Seaborn

LGTM. Can you add a NaCl-side test for the longjmp() bug to tests/longjmp/longjmp.c, please? https://codereview.chromium.org/923253002/diff/1/newlib/libc/machine/x86_64/setjmp.S ...

5 years, 10 months ago (2015-02-13 23:02:34 UTC) #2

Derek Schuff

Committed patchset #1 (id:1) manually as 2cc581b64e949a00839e666ba2a36e069830aaf4 (presubmit successful).

5 years, 10 months ago (2015-02-13 23:09:04 UTC) #3

Derek Schuff

5 years, 10 months ago (2015-02-13 23:31:32 UTC) #4

Message was sent while issue was closed.

https://codereview.chromium.org/923253002/diff/1/newlib/libc/machine/x86_64/s...
File newlib/libc/machine/x86_64/setjmp.S (right):

https://codereview.chromium.org/923253002/diff/1/newlib/libc/machine/x86_64/s...
newlib/libc/machine/x86_64/setjmp.S:8: * repository. It should be kept in sync.
On 2015/02/13 23:02:34, Mark Seaborn wrote:
> add "with minor modifications" (since there are some).

Done.

https://codereview.chromium.org/923253002/diff/1/newlib/libc/machine/x86_64/s...
newlib/libc/machine/x86_64/setjmp.S:67: jne     .Lskip
On 2015/02/13 23:02:34, Mark Seaborn wrote:
> PNaCl's version has ".skip".  Can you avoid unnecessary diffs, please, or else
> change PNaCl's version too?

I'll change PNaCl's version too. This version uses a local symbol to avoid
polluting the user's symbols. That doesn't matter for PNaCl's setjmp, but they
might as well agree.

Expand Messages | Collapse Messages