Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12181007:
add x32 ABI support (Closed)
|
Created:
7 years, 10 months ago by vapier Modified:
7 years, 8 months ago CC:
chromium-reviews, Markus (顧孟勤) Visibility:
Public. |
DescriptionAdd x32 ABI support
The x32 ABI has 4 byte longs/pointers, so the existing x86_64 code won't
work exactly as is. Many of the existing syscalls are unchanged (they
still take 64bit values), and the assembly code always has to fill the
64bit registers.
For the kernel stat structs, we need to mark all the fields explicitly
as 64bit rather than relying on 'long' to be 8 bytes. We end up having
to declare a dedicated statfs struct because it was being shared among
many other 32bit arches.
To make the syscall style easier, we end up gutting the existing syscall
macros and replacing them with a series of expandable ones (based on the
number of args). This uses an existing style as the mips code.
The default code now all casts everything to uintptr_t first (which will
be 32bit or 64bit based on the native size) and then casting it up to
64bits (which is required to fill a 64bit register). This works for the
vast majority of syscalls as they take native sized values.
However, there are a handful of syscalls that still take 64bit values.
We can't cast to uintptr_t first because it'd mean we'd truncate. We
also can't cast everything straight to 64bit because it'd incur a ton
of compile time warnings. So for these handful of syscalls, we expand
things ourselves using the new macros and take care of casting each arg
exactly as needed.
BUG=chromium:219015
TEST=compiled the code for x86_64 (64bit) & x86_64 (x32) & ran tests
Committed: https://code.google.com/p/linux-syscall-support/source/detail?r=19
Patch Set 1 #
Total comments: 13
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 22
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #
Total comments: 1
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1759: #define LSS_SC_ARG(a) (sizeof(a) == 8 ? \ "SC" is rather cryptic. Maybe "LSS_SYSCALL_ARG" instead? https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1761: (unsigned long long)(uintptr_t)(a)) I don't understand why you have a double cast here, and why it's conditional. Is it anything to do with avoiding warnings? Also, it might be clearer and more concise to use uint64_t rather than 'unsigned long long'. https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1763: #define LSS_SC_ARG(a) (long)(a) Should this be "((long)(a))"? https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1937: "r"(LSS_SC_ARG(fn)), "S"(LSS_SC_ARG(child_stack)), Line is >80 characters. Please put the arguments on separate lines. https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1939: "d"(LSS_SC_ARG(parent_tidptr)), "r"(LSS_SC_ARG(newtls)), Same here.
https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1759: #define LSS_SC_ARG(a) (sizeof(a) == 8 ? \ Done. https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1761: (unsigned long long)(uintptr_t)(a)) on x32, pointers are 32bit. syscall args are 64bit. you can't cast from a pointer type to a 64bit integer type straight or you'll get a lot of warnings. you might also get incorrect sign extension (i saw a bit of this when developing/testing, but i'd have to go back and come up with an exact example -- the mmap() test in the other CL shows it easily). so we bring in uintptr_t to take care of converting sanely (and warning free) from a pointer to an int, and then we cast it up to unsigned long long to get our 64bit quantity. we can't always cast the value to uintptr_t because that's a 32bit quantity and we could easily truncate an existing 64bit value which would be bad. i picked 'unsigned long long' simply because that was what the existing code uses. i can convert it to uint64_t. https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1763: #define LSS_SC_ARG(a) (long)(a) it could be. in practice it doesn't matter. i'll change it. https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1937: "r"(LSS_SC_ARG(fn)), "S"(LSS_SC_ARG(child_stack)), Done. https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1939: "d"(LSS_SC_ARG(parent_tidptr)), "r"(LSS_SC_ARG(newtls)), Done.
https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1761: (unsigned long long)(uintptr_t)(a)) On 2013/02/28 21:51:48, vapier wrote: > on x32, pointers are 32bit. syscall args are 64bit. you can't cast from a > pointer type to a 64bit integer type straight or you'll get a lot of warnings. I don't think using ?: is going to protect you from the warnings. If you're on x32, gcc will complain about the cast to "unsigned long long" being a size mismatch even if you want that side of the conditional to be compiled out. If you don't mind the compiler warnings (as you say in the commit message), then can't you do the following unconditionally? -- #define LS_SYSCALL_ARG(a) ((uint64_t)(a)) > you might also get incorrect sign extension (i saw a bit of this when > developing/testing, but i'd have to go back and come up with an exact example -- > the mmap() test in the other CL shows it easily). Hmm, won't casting to "unsigned long long" give you zero-extension?
https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1761: (unsigned long long)(uintptr_t)(a)) sure, i didn't mean to indicate it does entirely save me :). i highlighted in the commit message that this still warns. but using the uintptr_t cast cuts the # in half. unconditionally casting to (uint64_t) triggers the warnings i mentioned earlier: $ echo 'int main() { return (unsigned long long)main; }' | \ gcc -xc - -o /dev/null -Wall -mx32 -c <stdin>: In function ‘main’: <stdin>:1:21: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] unconditionally casting to uintptr_t avoids warnings, but silently truncates 64bit quantities :(
https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/1/lss/linux_syscall_support.h#n... lss/linux_syscall_support.h:1761: (unsigned long long)(uintptr_t)(a)) On 2013/03/01 23:21:49, vapier wrote: > sure, i didn't mean to indicate it does entirely save me :). i highlighted in > the commit message that this still warns. but using the uintptr_t cast cuts the > # in half. Having extra code that only reduces the number of warnings but doesn't eliminate them doesn't seem worthwhile to me. At least, if you're going to do that it needs a comment in the code. Is there a way you can fix all the warnings? In newer GCC versions you can suppress the warning with this: _Pragma("GCC diagnostic push"); /* Suppress complaints about casting a pointer to an integer of a different size. */ _Pragma("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); uint64_t x1 = (uint64_t) (void *) 0; _Pragma("GCC diagnostic pop"); The older GCC in Ubuntu Lucid only allows this _Pragma at top level, but if you're using x32 I expect you can assume a newer GCC.
this latest version implements your _Pragma idea. x32 is now warning free for me w/my tests. since x32 was first added to gcc-4.7, i think relying on that is OK.
https://codereview.chromium.org/12181007/diff/12001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/12001/lss/linux_syscall_support... lss/linux_syscall_support.h:1766: _Pragma("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ You also need to do "GCC diagnostic push"/"pop" otherwise this will leak out and disable the warning for the whole compilation unit. Maybe you want a macro like this: #define CAST_TO_UINT64(x) \ ({ uint64_t _tmp; \ _Pragma("GCC diagnostic push"); \ _Pragma("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ _tmp = (x); \ _Pragma("GCC diagnostic pop"); \ _tmp; \ }) I worry that this will fall foul of stricter GCC warnings that complain about GCC extensions like the "({...})" syntax. :-/ But maybe we shouldn't worry about those warnings. https://codereview.chromium.org/12181007/diff/12001/lss/linux_syscall_support... lss/linux_syscall_support.h:1769: #define LSS_SYSCALL_ARG(a) (sizeof(a) == 8 ? \ If you're disabling the warning, you don't need this conditional any more, right? LSS_SYSCALL_ARG(a) can be ((uint64_t)(a)) unconditionally.
you're right that the flags kick on for the rest of the file (and thus need the push/pop logic). i implemented your suggestion, and it works like 90% of the time. the remaining part i think i'm running into a gcc bug because it only warns when compiling from the .c file. if i preprocess it into an .i first and then compile that, i don't get the same warnings. i found i still need to cast first to uintptr_t else the value would get incorrectly extended. the most annoying part about this is that all this work is basically for 4 functions (the ones that use off_t). those are the only syscalls that i need to make sure stay 64bit because all the other fields are 32bit. i guess it's not feasible to try and special case those ?
On 8 March 2013 13:51, <vapier@chromium.org> wrote: > the most annoying part about this is that all this work is basically for 4 > functions (the ones that use off_t). those are the only syscalls that i > need to > make sure stay 64bit because all the other fields are 32bit. i guess it's > not > feasible to try and special case those ? > Oh, well, in that case, special casing these does seem quite reasonable. Which are the 4 functions in question? Just to check I understand this, off_t is 64-bit on x32, right? That seems reasonable (and is confirmed by http://lists.linuxtogo.org/pipermail/openembedded-core/2011-December/014015.html, a random reference I found by Googling :-) ). pread64()/pwrite64() are special-cased on ARM and MIPS. You might be able to use a similar pattern for x32. Cheers, Mark
i believe the exceptions are: fallocate ftruncate lseek mmap posix_fadvise preadv pwritev yes, x32 doesn't have any of the ugly i386 warts when it comes to LFS support. off_t and friends are all 64bits wide. C library calls like lseek64() are simply aliases to lseek(), and both x32 & x86_64 use the same syscall #. all the other funcs that might be weird (like ones dealing with time_t) aren't a problem because they use 32bit pointers to a 64bit type. which means we don't need to care. i'll see if i can special case these few entry points instead using the examples cited.
WDYT of this version ? passes my testsuite on x86_64 in both 64bit & x32 mode warning free.
Generally good -- some suggestions below. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (left): https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1850: __asm__ __volatile__("movq %5,%%r10; movq %6,%%r8; movq %7,%%r9;" \ FWIW, I find this somewhat easier to understand than the new more-heavily-macro-using version. Though there is some duplication, there is less for me to expand out in my head to check correctness of the inline asm. Maybe you could define something like this, which omits the casts: #define LSS_BODY4(type, name, arg1, arg2, arg3, arg4) \ long __res; \ __asm__ __volatile__("movq %5,%%r10;" LSS_ENTRYPOINT : \ "=a" (__res) : "0" (__NR_##name), \ "D" ((arg1)), "S" ((arg2)), "d" ((arg3)), \ "r" ((arg4)) : "r10", "r11", "rcx", "memory"); \ LSS_RETURN(type, __res); \ ... #define _syscall4(type,name,type1,arg1,type2,arg2,type3,arg3,type4,arg4) \ type LSS_NAME(name)(type1 arg1, type2 arg2, type3 arg3, type4 arg4) { \ LSS_BODY4(type, name, LSS_SYSCALL_ARG(arg1), ......) \ } What do you think? If you want to keep the macroified version I will look at it harder. :-) https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:412: unsigned long long st_dev; Maybe use uint64_t/int64_t in this struct for brevity and to be explicit? https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:526: #elif !defined(__x86_64__) Using "!defined" to put the catch-all case first is a little odd. How about "#if defined(__x86_64__)" and swap this definition with the "#else" definition? https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:544: /* cause any problems. Make sure these are 64-bit even on x32. */ Nit: you're mixing single-space and double-space sentence spacing in the same paragraph https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1828: #define _LSS_BODY(nr, type, name, cast, ...) \ Maybe 'nr' -> 'arg_count', otherwise it looks like nr could be the syscall number https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1838: #define LSS_BODY_ASM0 IIRC, there are some users of lss that #include linux_syscall_support.h multiple times with different #define settings. Yes... $ git grep SYS_LINUX_SYSCALL_SUPPORT_ ... third_party/tcmalloc/chromium/src/base/linuxthreads.cc: #undef SYS_LINUX_SYSCALL_SUPPORT_H third_party/tcmalloc/chromium/src/base/linuxthreads.cc: #undef SYS_LINUX_SYSCALL_SUPPORT_H (Argh.) This means that if you #define something new, you have to #undef it first to avoid redefinition warnings. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1992: /* Need to make sure off_t is passed as 64-bits even under x32. */ 'loff_t' -- see later comment https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:2885: /* Need to make sure off_t is passed as 64-bits even under x32. */ 'loff_t' https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:3553: /* Need to make sure off_t is passed as 64-bits even under x32. */ You mean loff_t here, since that's what these syscalls use. "even under x32" isn't quite right. Maybe "Need to make sure off_t is passed as 64-bits. Special-case this to avoid truncation on 32"?
https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (left): https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1850: __asm__ __volatile__("movq %5,%%r10; movq %6,%%r8; movq %7,%%r9;" \ i like the denser version myself because it's harder to have the different variants fall out of sync. this way, you fix it once in one place, and you're done. you don't have to worry about copying & pasting the same fix elsewhere. your suggestion would mitigate that somewhat compared to the code in the tree right now, but mine goes all the way :). https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:412: unsigned long long st_dev; Done. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:526: #elif !defined(__x86_64__) Done. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:544: /* cause any problems. Make sure these are 64-bit even on x32. */ Done. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1828: #define _LSS_BODY(nr, type, name, cast, ...) \ i used nr to match the existing mips style that exists below #define LSS_BODY(nr, type, name, args...) i can pick a diff name if you want https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1838: #define LSS_BODY_ASM0 i'll add the #undef, but i don't see how this can work In file included from test-skel.h:11:0, from lseek.c:1: ../lss/linux_syscall_support.h:166:8: error: redefinition of ‘struct kernel_dirent64’ In file included from test-skel.h:9:0, from lseek.c:1: ../lss/linux_syscall_support.h:166:8: note: originally defined here ... ../lss/linux_syscall_support.h:2715:1: error: redefinition of ‘sys_close’ In file included from test-skel.h:9:0, from lseek.c:1: ../lss/linux_syscall_support.h:2715:1: note: previous definition of ‘sys_close’ was here and those are just samples ... i see like ~100 redef errors https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1992: /* Need to make sure off_t is passed as 64-bits even under x32. */ Done. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:2885: /* Need to make sure off_t is passed as 64-bits even under x32. */ Done. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:3553: /* Need to make sure off_t is passed as 64-bits even under x32. */ Done.
LGTM. Can you run a Chromium try job before committing as with the previous change? https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1828: #define _LSS_BODY(nr, type, name, cast, ...) \ On 2013/03/25 20:21:08, vapier wrote: > i used nr to match the existing mips style that exists below > > #define LSS_BODY(nr, type, name, args...) > > i can pick a diff name if you want Ah, I didn't realise there is existing code that uses this pattern. Can you comment the refactoring in the commit message to explain that it follows those existing patterns? https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1838: #define LSS_BODY_ASM0 On 2013/03/25 20:21:08, vapier wrote: > i'll add the #undef, but i don't see how this can work > > In file included from test-skel.h:11:0, > from lseek.c:1: > ../lss/linux_syscall_support.h:166:8: error: redefinition of ‘struct > kernel_dirent64’ > In file included from test-skel.h:9:0, > from lseek.c:1: > ../lss/linux_syscall_support.h:166:8: note: originally defined here > ... > ../lss/linux_syscall_support.h:2715:1: error: redefinition of ‘sys_close’ > In file included from test-skel.h:9:0, > from lseek.c:1: > ../lss/linux_syscall_support.h:2715:1: note: previous definition of ‘sys_close’ > was here > > and those are just samples ... i see like ~100 redef errors Have a look at third_party/tcmalloc/chromium/src/base/linuxthreads.cc: namespace { class SysCalls { public: #define SYS_CPLUSPLUS #define SYS_ERRNO my_errno #define SYS_INLINE inline #define SYS_PREFIX -1 #undef SYS_LINUX_SYSCALL_SUPPORT_H #include "linux_syscall_support.h" SysCalls() : my_errno(0) { } int my_errno; }; } This is inside a namespace+class, so the struct redefinitions aren't redefinitions, only the preprocessor redefinitions are. :-) https://codereview.chromium.org/12181007/diff/27001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/27001/lss/linux_syscall_support... lss/linux_syscall_support.h:1856: #undef LSS_BODY_CLB0 How about CLB -> CLOBBER for readability?
i'll try and figure out the trybots again :) https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1828: #define _LSS_BODY(nr, type, name, cast, ...) \ Done. https://codereview.chromium.org/12181007/diff/20001/lss/linux_syscall_support... lss/linux_syscall_support.h:1838: #define LSS_BODY_ASM0 sneaky https://codereview.chromium.org/12181007/diff/27001/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/27001/lss/linux_syscall_support... lss/linux_syscall_support.h:1856: #undef LSS_BODY_CLB0 OK. i was trying to stick to 3 letters to match ASM and ARG, but that's really the only reason.
ok, this one passed CrOS/Linux trybots (didn't bother testing others) i seem to recall the commit button not working for LSS and i have to use dcommit
LGTM still On 2013/03/26 17:17:50, vapier wrote: > ok, this one passed CrOS/Linux trybots (didn't bother testing others) > > i seem to recall the commit button not working for LSS and i have to use dcommit Yes, you'll need to use dcommit.
Message was sent while issue was closed.
Committed patchset #9 manually as r19 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/12181007/diff/33002/lss/linux_syscall_support.h File lss/linux_syscall_support.h (right): https://codereview.chromium.org/12181007/diff/33002/lss/linux_syscall_support... lss/linux_syscall_support.h:2039: return (void (*)())(uintptr_t)res; I noticed that this breaks the NaCl build, which uses stricter warnings (-Wstrict-prototypes): In file included from src/trusted/service_runtime/linux/nacl_bootstrap.c:33: /mnt/data/b/build/slave/chromium-rel-linux-morenacl/build/src/third_party/lss/linux_syscall_support.h: In function 'sys_restore_rt': /mnt/data/b/build/slave/chromium-rel-linux-morenacl/build/src/third_party/lss/linux_syscall_support.h:2039: error: function declaration isn't a prototype [-Wstrict-prototypes] - from http://build.chromium.org/p/chromium.fyi/builders/More%20NaCl%20Tests%20%28li... So this should be "(void (*)(void))". Can you do a follow-on change to fix, please?
Message was sent while issue was closed.
done: https://codereview.chromium.org/13476002 |
