This is an archive of the discontinued LLVM Phabricator instance.

[Compiler-rt][MIPS] Fix cross build for XRAY
ClosedPublic

Authored by nitesh.jain on Sep 18 2017, 10:14 PM.

Details

Summary

The triple and target information (like ABI, Architecture revision) is not pass to XRAY_CFLAGS . This missing information in XRAY_CFLAGS causes cross build to fail. This patch fix it by appending CMAKE_CXX_FLAGS to XRAY_CFLAGS.

Diff Detail

Event Timeline

nitesh.jain created this revision.Sep 18 2017, 10:14 PM
sdardis requested changes to this revision.Sep 28 2017, 4:24 AM

I'm seeing a failure when attempting to run 'ninja check-xray'. The CMAKE_C_FLAGS are being appended twice / CMAKE_CXX_FLAGS and CMAKE_C_FLAGS are being appended to the commandline to build the unit-tests for xray, but the second set have '\' after each argument causing clang to reject the commandline.

This revision now requires changes to proceed.Sep 28 2017, 4:24 AM
nitesh.jain edited edge metadata.

Fix unit test build issues.

Hi Simon,

Please could you check whether it fixes unit test build ?

Thanks,
Nitesh

I'm a little bit confused here.

Are the CMAKE_C_FLAGS not included in the SANITIZER_COMMON_CFLAGS definition? If not, that's probably the right place to fix this.

Otherwise, I would have thought that instead of using CMAKE_C_FLAGS, maybe use CMAKE_CXX_FLAGS instead when building for cross-compilation?

Update diff as per suggestion

Update diff as per suggestion

I think you misunderstood my suggestion.

SANITIZER_COMMON_CFLAGS is defined somewhere else in compiler-rt. It already should be including the CMAKE_C_FLAGS into its definition, so I'm confused why we even need this change.

I still don't understand what problem you're trying to solve with this patch.

Can you clearly describe what the problem is, and why this patch is supposed to fix the problem? Why isn't CMAKE_CXX_FLAGS sufficient? Also, if this is in a hosted build, why not just set the target architecture for the compiler-rt build directly?

SANITIZER_COMMON_CFLAGS is defined somewhere else in compiler-rt. It already should be including the CMAKE_C_FLAGS into its definition, so I'm confused why we even need this change.

The CMAKE_C_FLAGS are not include in SANITIZER_COMMON_CFLAGS because of which cross-native build of x-ray is failing.

-Nitesh Jain

SANITIZER_COMMON_CFLAGS is defined somewhere else in compiler-rt. It already should be including the CMAKE_C_FLAGS into its definition, so I'm confused why we even need this change.

The CMAKE_C_FLAGS are not include in SANITIZER_COMMON_CFLAGS because of which cross-native build of x-ray is failing.

Okay, but can't you use CMAKE_CXX_FLAGS instead of CMAKE_C_FLAGS?

Other question is why CMAKE_C_FLAGS is important and special here.

nitesh.jain added a reviewer: dberris.

Updated diff as per suggestion

dberris edited edge metadata.Oct 10 2017, 5:53 AM

The patch seems ill-formed -- usually you want to create the patch from the root of the project. In this case it will be from the root of the compiler-rt repository/working copy.

CMakeLists.txt
273 ↗(On Diff #118327)

Do you still really need this if you just use cmake -DCMAKE_CXX_FLAGS=... without this change? What happens when you revert this change and provide the target flags instead when you invoke cmake?

nitesh.jain added inline comments.Oct 10 2017, 6:18 AM
CMakeLists.txt
273 ↗(On Diff #118327)

Without this change I am getting following errors

[3/788] Building ASM object projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-mipsel.dir/xray_trampoline_mips.S.o
FAILED: projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-mipsel.dir/xray_trampoline_mips.S.o
/home/nin/LLVM/x86_build/bin/clang -DXRAY_HAS_EXCEPTIONS=1 -DXRAY_SUPPORTED=1 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/xray -I/home/nin/LLVM/llvm/projects/compiler-rt/lib/xray -Iinclude -I/home/nin/LLVM/llvm/include -I/home/nin/LLVM/llvm/projects/compiler-rt/lib/xray/.. -I/home/nin/LLVM/llvm/projects/compiler-rt/lib/xray/../../include -O3 -DNDEBUG -fno-lto -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fvisibility-inlines-hidden -fno-lto -O3 -gline-tables-only -MD -MT projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-mipsel.dir/xray_trampoline_mips.S.o -MF projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-mipsel.dir/xray_trampoline_mips.S.o.d -o projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-mipsel.dir/xray_trampoline_mips.S.o -c /home/nin/LLVM/llvm/projects/compiler-rt/lib/xray/xray_trampoline_mips.S

clang -cc1as: error: unknown target triple 'x86_64-unknown-linux-gnu', please use -triple or -arch

My build commands is

cmake -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE='' -DCMAKE_C_FLAGS="$cflags" -DCMAKE_CXX_FLAGS="$cflags -std=c++11" -DLLVM_TABLEGEN=$LLVM_TBLGEN -DCLANG_TABLEGEN=$CLANG_TBLGEN -DCMAKE_CROSSCOMPILING=1 -DLLVM_TARGETS_TO_BUILD=Mips -DLLVM_DEFAULT_TARGET_TRIPLE=$TRIPLE -DCMAKE_C_COMPILER_TARGET=$TRIPLE -DCMAKE_CXX_COMPILER_TARGET=$TRIPLE -DLLVM_HOST_TRIPLE=$TRIPLE -DLLVM_TARGET_ARCH=Mips -DCOMPILER_RT_DEFAULT_TARGET_ONLY=1 -DCOMPILER_RT_DEFAULT_TARGET_ARCH=mipsel -DCOMPILER_RT_TEST_COMPILER=$cxx_bin -DCMAKE_FIND_ROOT_PATH=$sysroot -DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM=BOTH -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY -DCMAKE_C_COMPILER=$cc_bin -DCMAKE_CXX_COMPILER=$cxx_bin

cflags=--gcc-toolchain=/projects/mipssw/toolchains/mips-mti-linux-gnu/2015.01-7/ --target=mipsel-mti-linux-gnu -mabi=32 -EL -mips32r2 -latomic.

Let me know if I miss anything

Thanks

dberris added inline comments.
CMakeLists.txt
273 ↗(On Diff #118327)

Aha -- this is much better, thanks.

I think I see what the problem is here, and it might be better to fix this a different way.

There's a change I prepared a while back that changes the way the runtime is built (https://reviews.llvm.org/D37372#65c567b0) which I've since abandoned. That's isolated to the XRay CMakeLists.txt file in compiler-rt, which might be a better definition conducive to cross-compilation.

I see that it might be useful to use COMPILER_RT_DEFAULT_TARGET_ARCH somehow if we're not already using it in the compiler-rt cmake helpers.

@beanz might know better how to support cross-compiler set-ups too. @eugenis might also know more about how the sanitizers achieve this better.

eugenis added inline comments.Oct 10 2017, 1:30 PM
CMakeLists.txt
273 ↗(On Diff #118327)

I think you are right. add_compiler_rt* functions should take care of this on their own by using TARGET_{$ARCH}_FLAGS which should include CMAKE_C_FLAGS / CMAKE_CXX_FLAGS.

ddunbar resigned from this revision.Oct 12 2017, 8:32 AM
nitesh.jain edited the summary of this revision. (Show Details)

Update diff as per suggestion.

Thanks

dberris accepted this revision.Oct 16 2017, 12:59 PM

For XRay, I think this is fine. Although I suspect it would be better to do this in the definition of add_compiler_rt_runtime and other add_compiler_rt_* functions instead.

I'll leave it up to you to decide whether you want to change the definitions of the helpers.

lib/xray/CMakeLists.txt
64–67

I think you should just be able to check whether TARGET_FLAGS is defined, regardless of whether CMAKE_CXX_FLAGS is defined or not.

nitesh.jain marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.