Page MenuHomePhabricator

[libunwind] Set LIBUNWIND_ASM_SOURCES to the ASM source language from C
ClosedPublic

Authored by JamesNagurne on Jan 17 2020, 12:49 PM.

Details

Summary
I believe this is an oversight from the import of libunwind into its own
library from libc++abi.

In libc++abi, these files had the .s suffix, which indicates that the file
is a preprocessed assembly source file. This caused problems because the
files rely upon preprocessors to guard target-specific blocks.

To fix this, the CMakeLists file marked these files as C so that the
preprocessor would be run over them, but then the compiler would correctly
identify the files as assembly and compile them as such.

When imported to libunwind, these files were (correctly) renamed with .S
suffixes, which are non-preprocessed assembly. Thus, we no longer need the
C language property.

The benefit here is that the files can now benefit from CMAKE_ASM_FLAGS
rather than CMAKE_C_FLAGS.

Diff Detail

Event Timeline

JamesNagurne created this revision.Jan 17 2020, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2020, 12:49 PM
smeenai added subscribers: compnerd, smeenai.

Adding @compnerd, who's familiar with both libunwind and CMake :)

libunwind/src/CMakeLists.txt
26–27

Do you need this at all, or will CMake just do the right thing by default?

JamesNagurne added inline comments.
libunwind/src/CMakeLists.txt
26–27

I thought about that. I think it will do the right thing, but I was paranoid :)

I'll get rid of it, run it with a dummy CMAKE, and update

Confirmed that cmake correctly identifies '.S' as ASM, and uses CMAKE_ASM_FLAGS

smeenai accepted this revision.Jan 17 2020, 3:36 PM

The diff here looks weird ... it looks like a diff against your previous diff instead of a diff against master? (Phabricator says the line you're deleting has LANGUAGE ASM instead of LANGUAGE C, as it does on master.)

The diff here looks weird ... it looks like a diff against your previous diff instead of a diff against master? (Phabricator says the line you're deleting has LANGUAGE ASM instead of LANGUAGE C, as it does on master.)

Eek, sorry. We use CodeCollaborator internally, which would have effectively shown the diff from the original commit to the final (after 2 change sets), which would have shown the C in the before and nothing after. I mistakenly treated Phabricator the same way erroneously.

I'll re-upload this so it looks right. My apologies!

Updated to reflect master -> diff 2, rather than diff 1 -> diff 2

The diff here looks weird ... it looks like a diff against your previous diff instead of a diff against master? (Phabricator says the line you're deleting has LANGUAGE ASM instead of LANGUAGE C, as it does on master.)

Eek, sorry. We use CodeCollaborator internally, which would have effectively shown the diff from the original commit to the final (after 2 change sets), which would have shown the C in the before and nothing after. I mistakenly treated Phabricator the same way erroneously.

I'll re-upload this so it looks right. My apologies!

No worries! LGTM.

phosek accepted this revision.Jan 24 2020, 7:11 PM

LGTM

This revision is now accepted and ready to land.Jan 24 2020, 7:11 PM
This revision was automatically updated to reflect the committed changes.

This broke my build, for both mingw and linux, with both cmake 3.10.2 and 3.16.2 - the assembly files don't get built at all. Reproducible on linux with a plain "cmake .." (in llvm-project/libunwind/build) without further args.

Is some custom build config needed for this to work?

I'll go ahead and revert this later today to unbreak my build.

This broke my build, for both mingw and linux, with both cmake 3.10.2 and 3.16.2 - the assembly files don't get built at all. Reproducible on linux with a plain "cmake .." (in llvm-project/libunwind/build) without further args.

Is some custom build config needed for this to work?

Ok, so it seems this works when built via the main llvm cmakefile, but not standalone. So I guess the libunwind cmakefiles needs some extras for hooking this up, that are included only when built standalone.

This broke my build, for both mingw and linux, with both cmake 3.10.2 and 3.16.2 - the assembly files don't get built at all. Reproducible on linux with a plain "cmake .." (in llvm-project/libunwind/build) without further args.

Is some custom build config needed for this to work?

Ok, so it seems this works when built via the main llvm cmakefile, but not standalone. So I guess the libunwind cmakefiles needs some extras for hooking this up, that are included only when built standalone.

I fixed this in https://reviews.llvm.org/rG0e0c65264aeb6f66b6f711884c55cdbf66d975f6, but building assembly files as assembly instead of C breaks building for mingw, for which I sent an additional patch at D73436, essentially conditionally reverting this change for mingw targets.

This broke my build, for both mingw and linux, with both cmake 3.10.2 and 3.16.2 - the assembly files don't get built at all. Reproducible on linux with a plain "cmake .." (in llvm-project/libunwind/build) without further args.

Is some custom build config needed for this to work?

Ok, so it seems this works when built via the main llvm cmakefile, but not standalone. So I guess the libunwind cmakefiles needs some extras for hooking this up, that are included only when built standalone.

I fixed this in https://reviews.llvm.org/rG0e0c65264aeb6f66b6f711884c55cdbf66d975f6, but building assembly files as assembly instead of C breaks building for mingw, for which I sent an additional patch at D73436, essentially conditionally reverting this change for mingw targets.

Apologies for the breakage! I did look at the project command at some point and wondered if I needed to populate it with languages, but decided that maybe I didn't after CMAKE_ASM_FLAGS worked for my llvm-runtimes build.
I'm glad to see a fix was able to be made without reverting the intent of the change.

This change also breaks MacOS, as -arch doesn't get passed to these invocations. Possibly CMAKE_OSX_ARCHITECTURES not being handled correctly within CMake.

Confirmed that CMake has a bug here: https://github.com/Kitware/CMake/blob/master/Source/cmLocalGenerator.cxx#L1780

Given this I'm going to add APPLE in addition to MINGW.