Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
411803002:
Fix for building ARM/linux using clang. (Closed)
|
Created:
6 years, 5 months ago by Sam Clegg Modified:
6 years, 4 months ago CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionFix for building ARM/linux using clang.
This fix removes the use of 'vfpcc' which as an unknown
register in clang, and instead uses __builtin_arm_set_fpscr.
BUG= https://code.google.com/p/chromium/issues/detail?id=395832
R=jfb@chromium.org, mseaborn@chromium.org
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=13556
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : use __builtin_arm_set_fpscr on clang. #Patch Set 5 : #
Total comments: 3
Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 18 (0 generated)
regarding the munger, its seems that clang has a different order/number of segments. Is this to be expected? How should we handle it?
What steps did you do to build with Clang? I'd like to try this myself so that I can see the ELF headers that nacl_helper_bootstrap gets. https://codereview.chromium.org/411803002/diff/1/src/include/concurrency_ops.h File src/include/concurrency_ops.h (right): https://codereview.chromium.org/411803002/diff/1/src/include/concurrency_ops.... src/include/concurrency_ops.h:91: __builtin___clear_cache((char*)writable_addr, (char*)writable_addr + size); What version of Clang did you test against? It looks like Clang has been fixed to use void* and so match GCC: http://llvm.org/viewvc/llvm-project?view=revision&revision=181810 https://codereview.chromium.org/411803002/diff/1/src/trusted/service_runtime/... File src/trusted/service_runtime/linux/nacl_bootstrap.gyp (right): https://codereview.chromium.org/411803002/diff/1/src/trusted/service_runtime/... src/trusted/service_runtime/linux/nacl_bootstrap.gyp:191: 'compiler': '<!(echo ${CXX:=arm-linux-gnueabi-g++})', I don't understand this part of the change. Can you explain?
Added a link to the chrome side changes needed to build with clang. https://codereview.chromium.org/411803002/diff/1/src/include/concurrency_ops.h File src/include/concurrency_ops.h (right): https://codereview.chromium.org/411803002/diff/1/src/include/concurrency_ops.... src/include/concurrency_ops.h:91: __builtin___clear_cache((char*)writable_addr, (char*)writable_addr + size); On 2014/07/23 19:13:41, Mark Seaborn wrote: > What version of Clang did you test against? > > It looks like Clang has been fixed to use void* and so match GCC: > http://llvm.org/viewvc/llvm-project?view=revision&revision=181810 clang is coming from the chrome tree: third_party/llvm-build/Release+Asserts I assume its very recent. https://codereview.chromium.org/411803002/diff/1/src/trusted/service_runtime/... File src/trusted/service_runtime/linux/nacl_bootstrap.gyp (right): https://codereview.chromium.org/411803002/diff/1/src/trusted/service_runtime/... src/trusted/service_runtime/linux/nacl_bootstrap.gyp:191: 'compiler': '<!(echo ${CXX:=arm-linux-gnueabi-g++})', On 2014/07/23 19:13:41, Mark Seaborn wrote: > I don't understand this part of the change. Can you explain? This action needs access to the target linker. I'm can't remember exactly why. Previously we were setting $CXX when cross compiling. Now we are using clang with a "-target" argument, so CXX is not set. What is more g++ (the default) or clang where would both give the wrong result as they would default to host builds. I admit that this is pretty horrible. Perhaps there is some way we can avoid this action. I will investigate.
https://codereview.chromium.org/411803002/diff/1/src/include/concurrency_ops.h File src/include/concurrency_ops.h (right): https://codereview.chromium.org/411803002/diff/1/src/include/concurrency_ops.... src/include/concurrency_ops.h:91: __builtin___clear_cache((char*)writable_addr, (char*)writable_addr + size); On 2014/07/23 19:21:52, Sam Clegg wrote: > On 2014/07/23 19:13:41, Mark Seaborn wrote: > > What version of Clang did you test against? > > > > It looks like Clang has been fixed to use void* and so match GCC: > > http://llvm.org/viewvc/llvm-project?view=revision&revision=181810 > > clang is coming from the chrome tree: third_party/llvm-build/Release+Asserts > > I assume its very recent. Ah, the issue is that __clear_cache() and __builtin___clear_cache() have different types: include/clang/Basic/Builtins.def:BUILTIN(__builtin___clear_cache, "vc*c*", "n") include/clang/Basic/BuiltinsARM.def:BUILTIN(__clear_cache, "vv*v*", "i") It should be OK to change this to use __clear_cache() instead. Aside from the type, they appear to behave the same on both GCC and Clang. They both call libgcc's __clear_cache(). Can you do a standalone change to change this part?
https://codereview.chromium.org/411803002/diff/1/src/trusted/service_runtime/... File src/trusted/service_runtime/linux/nacl_bootstrap_munge_phdr.py (right): https://codereview.chromium.org/411803002/diff/1/src/trusted/service_runtime/... src/trusted/service_runtime/linux/nacl_bootstrap_munge_phdr.py:32: segment_num = '3' I investigated this. I was able to reproduce the problem, and I got the following ELF headers: $ readelf -a scons-out/dbg-linux-arm/obj/src/trusted/service_runtime/nacl_bootstrap_raw ... Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align EXIDX 0x0029e8 0x000129e8 0x000129e8 0x00008 0x00008 R 0x4 LOAD 0x000000 0x00010000 0x00010000 0x029f0 0x029f0 R E 0x1000 LOAD 0x0029f0 0x000139f0 0x000139f0 0x00000 0x01005 RW 0x1000 LOAD 0x003000 0x00015000 0x00015000 0x00000 0x3ffed000 0x1000 LOAD 0x003000 0x40002000 0x40002000 0x00014 0x00014 RW 0x1000 NOTE 0x000114 0x00010114 0x00010114 0x00024 0x00024 R 0x4 GNU_STACK 0x000000 0x00000000 0x00000000 0x00000 0x00000 RW 0x4 Section to Segment mapping: Segment Sections... 00 .ARM.exidx 01 .note.gnu.build-id .text .rodata .ARM.exidx 02 .bss 03 .reserve 04 .r_debug 05 .note.gnu.build-id 06 ... ARM.exidx is unwind info in an ARM-specific format. It seems Clang generates this even if we compile with -fno-exceptions (the default for C anyway) and -fno-unwind-tables. A simple fix is just to change the "DISCARD" list in the linker script: --- a/src/trusted/service_runtime/linux/nacl_bootstrap.x +++ b/src/trusted/service_runtime/linux/nacl_bootstrap.x @@ -131,5 +131,6 @@ SECTIONS { *(.reginfo) *(.rel*) *(.igot.plt) + *(.ARM.exidx) } }
https://codereview.chromium.org/411803002/diff/40001/src/trusted/service_runt... File src/trusted/service_runtime/nacl_syscall_common.c (right): https://codereview.chromium.org/411803002/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/nacl_syscall_common.c:897: /*__asm__ volatile("fmxr fpscr, %0" :: "r" (0xdeadbeef) : "vfpcc");*/ Try __builtin_arm_set_fpscr.
OK, I think the remaining two changes are good to submit. PTAL.
ARM change lgtm
ping. This is needed so we can have a working arm bot once again.
https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... File src/trusted/service_runtime/linux/nacl_bootstrap.gyp (right): https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... src/trusted/service_runtime/linux/nacl_bootstrap.gyp:165: 'compiler': '<!(echo ${CXX:=arm-linux-gnueabi-g++})', I don't really understand what's going on here, but hard-coding this here seems wrong. It's as likely to be arm-linux-gnueabihf-g++, for example.
https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... File src/trusted/service_runtime/linux/nacl_bootstrap.gyp (right): https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... src/trusted/service_runtime/linux/nacl_bootstrap.gyp:162: # this case $CC and $CXX are not set in the envrionement). "environment" https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... src/trusted/service_runtime/linux/nacl_bootstrap.gyp:165: 'compiler': '<!(echo ${CXX:=arm-linux-gnueabi-g++})', It's a bit counterintuitive that in order to build with clang, you have to specify arm-gcc here. Would this then be the only place in the Gyp build where arm-gcc gets invoked? And would this be the only part that requires the arm-gcc Debian package to be installed? Is this only needed in conjunction with another change (like https://codereview.chromium.org/414343003/)? Could we get this to use the same host compiler that Gyp uses? Is there any way to refer to CC from 'make_global_settings'? If not, could we add another variable to common.gypi to define this compiler? If this is tricky to resolve, I'd suggest sending the nacl_syscall_common.c change as a separate change.
On 2014/07/28 19:23:20, Mark Seaborn wrote: > https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... > File src/trusted/service_runtime/linux/nacl_bootstrap.gyp (right): > > https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... > src/trusted/service_runtime/linux/nacl_bootstrap.gyp:162: # this case $CC and > $CXX are not set in the envrionement). > "environment" > > https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... > src/trusted/service_runtime/linux/nacl_bootstrap.gyp:165: 'compiler': '<!(echo > ${CXX:=arm-linux-gnueabi-g++})', > It's a bit counterintuitive that in order to build with clang, you have to > specify arm-gcc here. > > Would this then be the only place in the Gyp build where arm-gcc gets invoked? > And would this be the only part that requires the arm-gcc Debian package to be > installed? > Its a little counter intuitive yes. However we do already require the arm-linux-gnueabihf toolchain to be installed for the assembler (we use -no-integrated-assembler to clagn) and the linker. > Is this only needed in conjunction with another change (like > https://codereview.chromium.org/414343003/) Its also needed if we want to switch to clang.. which we do. Remember we are only settings sensible default here. Setting $CXX can still override this. An alternative would be to default to "clang -target arm-linux-gnueabihf". If we default to simple 'g++' or 'clang' we the linking of nacl_bootstrap_raw generates: /usr/bin/ld.bfd: unrecognised emulation mode: armelf_linux_eabi > Could we get this to use the same host compiler that Gyp uses? Is there any way > to refer to CC from 'make_global_settings'? If not, could we add another > variable to common.gypi to define this compiler? Ideally we could do this yes, there is even a TODO above to do it, but there is no way to reference the compiler name or make_global_settings in conditionals AFAICT. > If this is tricky to resolve, I'd suggest sending the nacl_syscall_common.c > change as a separate change. Might do that yes, although we can't move forward without both fixes.
On 2014/07/28 19:20:51, Roland McGrath wrote: > https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... > File src/trusted/service_runtime/linux/nacl_bootstrap.gyp (right): > > https://codereview.chromium.org/411803002/diff/80001/src/trusted/service_runt... > src/trusted/service_runtime/linux/nacl_bootstrap.gyp:165: 'compiler': '<!(echo > ${CXX:=arm-linux-gnueabi-g++})', > I don't really understand what's going on here, but hard-coding this here seems > wrong. It's as likely to be arm-linux-gnueabihf-g++, for example. I admit this code gyp file is horrible, however its only hard-coding a sensible default which is override-able with $CXX. For what its worth the 'g++' in the block below is just as hardcoded and just a likely to be wrong. In fact thats what required this change: /usr/bin/ld.bfd: unrecognised emulation mode: armelf_linux_eabi
Split into separate issue: https://codereview.chromium.org/428753002/
On 2014/07/28 21:31:38, Sam Clegg wrote: > Split into separate issue: https://codereview.chromium.org/428753002/ Mark, can I get an OWNER LG on this, now that the more controversial change is split out.
On 2014/07/28 21:46:55, Sam Clegg wrote: > Mark, can I get an OWNER LG on this, now that the more controversial > change is split out. Please set BUG= https://code.google.com/p/chromium/issues/detail?id=395832. Maybe update the commit message to reflect the current change? Then LGTM, thanks.
On 2014/07/29 17:52:08, Mark Seaborn wrote: > On 2014/07/28 21:46:55, Sam Clegg wrote: > > Mark, can I get an OWNER LG on this, now that the more controversial > > change is split out. > > Please set BUG= https://code.google.com/p/chromium/issues/detail?id=395832. > Maybe update the commit message to reflect the current change? Then LGTM, > thanks. Added BUG=. I think I updated the CL description yesterday. Is there something in particular you think should be changed.
Message was sent while issue was closed.
Committed patchset #7 manually as r13556 (presubmit successful). |
