Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
389673002:
MIPS: The new ABI should be allowed to use r8 and r9 for syscalls. (Closed)
|
Created:
6 years, 5 months ago by Lei Zhang Modified:
6 years, 4 months ago CC:
chromium-reviews, Markus (顧孟勤) Visibility:
Public. |
DescriptionMIPS: The new ABI should be allowed to use r8 and r9 for syscalls.
BUG=linux-syscall-support:7
R=mseaborn@chromium.org, petarj@mips.com
Committed: https://code.google.com/p/linux-syscall-support/source/detail?r=31
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : address comments, remove tabs #Patch Set 4 : for committing, just rebase #Messages
Total messages: 15 (0 generated)
See comment on line 2613 in the original file.
On 2014/07/11 18:55:47, Lei Zhang wrote: > See comment on line 2613 in the original file. Can you put some explanation into the commit message, please, so that someone who's not familiar with MIPS syscall calling conventions can understand this change? Can you also add a TEST= field to say how you tested this?
On 2014/07/11 23:21:34, Mark Seaborn wrote: > On 2014/07/11 18:55:47, Lei Zhang wrote: > > See comment on line 2613 in the original file. > > Can you put some explanation into the commit message, please, so that someone > who's not familiar with MIPS syscall calling conventions can understand this > change? > > Can you also add a TEST= field to say how you tested this? Updated the description.
[CC'ing Petar about the hi/lo issue and for general review.] https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h#newc... linux_syscall_support.h:2585: : "$10", "$11", "$12", "$13", "$14", \ So, you're only changing the list of clobbered registers, to remove registers that are used as input constraints if _MIPS_SIM != _MIPS_SIM_ABI32 below. Would it make sense to conditionally #define a LSS_SYSCALL_CLOBBERS to reduce the duplication here? Does that mean that compilation was failing with the error "‘asm’ operand has impossible constraints" from GCC? If so, can you say that in the commit message? It looks like the N32/N64 support in this file wouldn't have worked before, unless the compiler was more lax before. Does your TEST= line mean you only compile-tested this, and didn't run the code? For comparison, I noticed that glibc uses: #define __SYSCALL_CLOBBERS "$1", "$3", "$10", "$11", "$12", "$13", \ "$14", "$15", "$24", "$25", "hi", "lo", "memory" (from ports/sysdeps/unix/sysv/linux/mips/mips64/n32/sysdep.h) Does that mean this code is missing clobbers for the "hi" and "lo" registers? See: https://sourceware.org/ml/libc-ports/2009-09/msg00006.html ("Avoid reuse of MIPS "hi" and "lo"") http://www.linux-mips.org/wiki/Syscall
https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h#newc... linux_syscall_support.h:2585: : "$10", "$11", "$12", "$13", "$14", \ On 2014/07/12 23:58:08, Mark Seaborn wrote: > So, you're only changing the list of clobbered registers, to remove registers > that are used as input constraints if _MIPS_SIM != _MIPS_SIM_ABI32 below. > > Would it make sense to conditionally #define a LSS_SYSCALL_CLOBBERS to reduce > the duplication here? Disclaimer: I've never worked on this before. So it's your call. > Does that mean that compilation was failing with the error "‘asm’ operand has > impossible constraints" from GCC? If so, can you say that in the commit > message? Done. It was "error: asm-specifier for variable '__r8' conflicts with asm clobber list" > It looks like the N32/N64 support in this file wouldn't have worked before, > unless the compiler was more lax before. No idea if anyone's ever tested this before. > Does your TEST= line mean you only compile-tested this, and didn't run the code? There's other issues in the Breakpad build, and I don't have a MIPS VM set up. I can if you'd like. > For comparison, I noticed that glibc uses: > > #define __SYSCALL_CLOBBERS "$1", "$3", "$10", "$11", "$12", "$13", \ > "$14", "$15", "$24", "$25", "hi", "lo", "memory" > > (from ports/sysdeps/unix/sysv/linux/mips/mips64/n32/sysdep.h) > > Does that mean this code is missing clobbers for the "hi" and "lo" registers? > > See: > https://sourceware.org/ml/libc-ports/2009-09/msg00006.html ("Avoid reuse of MIPS > "hi" and "lo"") > http://www.linux-mips.org/wiki/Syscall I defer to petarj.
On 14 July 2014 12:39, <thestig@chromium.org> wrote: > > https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h > File linux_syscall_support.h (right): > > https://codereview.chromium.org/389673002/diff/1/linux_ > syscall_support.h#newcode2585 > linux_syscall_support.h:2585: : "$10", "$11", "$12", "$13", "$14", > \ > On 2014/07/12 23:58:08, Mark Seaborn wrote: > >> So, you're only changing the list of clobbered registers, to remove >> registers > > that are used as input constraints if _MIPS_SIM != _MIPS_SIM_ABI32 below. > > > Would it make sense to conditionally #define a LSS_SYSCALL_CLOBBERS to >> reduce > > the duplication here? >> > > Disclaimer: I've never worked on this before. So it's your call. Yeah, if you don't mind, I think it would be clearer to add a macro for that. > > Does your TEST= line mean you only compile-tested this, and didn't run >> > the code? > > There's other issues in the Breakpad build, and I don't have a MIPS VM > set up. I can if you'd like. No, you don't have to. I just wanted to check what the level of testing was. > For comparison, I noticed that glibc uses: >> > > #define __SYSCALL_CLOBBERS "$1", "$3", "$10", "$11", "$12", "$13", \ >> "$14", "$15", "$24", "$25", "hi", "lo", "memory" >> > > (from ports/sysdeps/unix/sysv/linux/mips/mips64/n32/sysdep.h) >> > > Does that mean this code is missing clobbers for the "hi" and "lo" >> registers? > > > See: >> https://sourceware.org/ml/libc-ports/2009-09/msg00006.html ("Avoid reuse >> of MIPS > > "hi" and "lo"") >> http://www.linux-mips.org/wiki/Syscall >> > > I defer to petarj. No worries, I don't expect you to fix this existing problem. :-) Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h#newc... linux_syscall_support.h:2585: : "$10", "$11", "$12", "$13", "$14", \ On 2014/07/14 19:39:28, Lei Zhang wrote: > There's other issues in the Breakpad build, and I don't have a MIPS VM set up. I > can if you'd like. breakpad does not support mips64 yet. > > Does that mean this code is missing clobbers for the "hi" and "lo" registers? > > True. Mimicking __SYSCALL_CLOBBERS from ./sysdeps/unix/sysv/linux/mips/mips64/n64/sysdep.h is the way to go, I agree with Mark.
See patch set 2. - Added LSS_SYSCALL_CLOBBERS - Mimics __SYSCALL_CLOBBERS https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h#newc... linux_syscall_support.h:2585: : "$10", "$11", "$12", "$13", "$14", \ On 2014/07/16 02:23:24, petarj wrote: > On 2014/07/14 19:39:28, Lei Zhang wrote: > > There's other issues in the Breakpad build, and I don't have a MIPS VM set up. > I > > can if you'd like. > > breakpad does not support mips64 yet. > > > > Does that mean this code is missing clobbers for the "hi" and "lo" > registers? > > > > > True. Mimicking __SYSCALL_CLOBBERS from > ./sysdeps/unix/sysv/linux/mips/mips64/n64/sysdep.h > is the way to go, I agree with Mark. Done.
On 2014/07/16 23:42:07, Lei Zhang wrote: > See patch set 2. > > - Added LSS_SYSCALL_CLOBBERS > - Mimics __SYSCALL_CLOBBERS > > https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h > File linux_syscall_support.h (right): > > https://codereview.chromium.org/389673002/diff/1/linux_syscall_support.h#newc... > linux_syscall_support.h:2585: : "$10", "$11", "$12", "$13", "$14", \ > On 2014/07/16 02:23:24, petarj wrote: > > On 2014/07/14 19:39:28, Lei Zhang wrote: > > > There's other issues in the Breakpad build, and I don't have a MIPS VM set > up. > > I > > > can if you'd like. > > > > breakpad does not support mips64 yet. > > > > > > Does that mean this code is missing clobbers for the "hi" and "lo" > > registers? > > > > > > > > True. Mimicking __SYSCALL_CLOBBERS from > > ./sysdeps/unix/sysv/linux/mips/mips64/n64/sysdep.h > > is the way to go, I agree with Mark. > > Done. lgtm
Mark: If you are ok with the CL, please commit it on my behalf.
On 2014/07/17 22:31:05, Lei Zhang wrote: > Mark: If you are ok with the CL, please commit it on my behalf. I'll give you commit rights so you can commit the change yourself. https://codereview.chromium.org/389673002/diff/20001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/389673002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2570: #define LSS_SYSCALL_CLOBBERS "$8", "$9", "$10", "$11", "$12", \ Shouldn't this also have $1 (aka $at), $3 (aka $v1), hi and lo? AFAICT, these parts are the same for both old and new calling conventions. https://codereview.chromium.org/389673002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2574: #define LSS_SYSCALL_CLOBBERS "$1", "3" "$10", "$11", "$12", \ Presumably should be: "$3", i.e. missing comma and "$".
https://codereview.chromium.org/389673002/diff/20001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/389673002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2570: #define LSS_SYSCALL_CLOBBERS "$8", "$9", "$10", "$11", "$12", \ On 2014/07/22 18:19:37, Mark Seaborn wrote: > Shouldn't this also have $1 (aka $at), $3 (aka $v1), hi and lo? > > AFAICT, these parts are the same for both old and new calling conventions. Done. https://codereview.chromium.org/389673002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2574: #define LSS_SYSCALL_CLOBBERS "$1", "3" "$10", "$11", "$12", \ On 2014/07/22 18:19:37, Mark Seaborn wrote: > Presumably should be: > "$3", > > i.e. missing comma and "$". Done.
LGTM, thanks. Can you roll this into Chromium after committing, please, as per https://code.google.com/p/linux-syscall-support/wiki/HowToContribute?
Message was sent while issue was closed.
Committed patchset #4 manually as r31 (presubmit successful).
Message was sent while issue was closed.
On 2014/08/01 00:49:06, Mark Seaborn wrote: > LGTM, thanks. Can you roll this into Chromium after committing, please, as per > https://code.google.com/p/linux-syscall-support/wiki/HowToContribute Done, https://codereview.chromium.org/435853002 |
