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

[JamesNagurne]

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.

[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]

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

[JamesNagurne]

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

[smeenai]

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.)

[JamesNagurne]

In D72952#1827668, @smeenai wrote:

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!

[JamesNagurne]

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

[smeenai]

In D72952#1827678, @JamesNagurne wrote:

In D72952#1827668, @smeenai wrote:

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]

LGTM

[mstorsjo]

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.

[mstorsjo]

In D72952#1840794, @mstorsjo wrote:

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.

[mstorsjo]

In D72952#1840841, @mstorsjo wrote:

In D72952#1840794, @mstorsjo wrote:

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.

[JamesNagurne]

In D72952#1841048, @mstorsjo wrote:

In D72952#1840841, @mstorsjo wrote:

In D72952#1840794, @mstorsjo wrote:

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.

[Bigcheese]

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

[Bigcheese]

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.

libunwind/src/CMakeLists.txt

set(LIBUNWIND_C_SOURCES
     )
 set_source_files_properties(${LIBUNWIND_C_SOURCES}
                             PROPERTIES
                               COMPILE_FLAGS "-std=c99")
 
 set(LIBUNWIND_ASM_SOURCES
     UnwindRegistersRestore.S
     UnwindRegistersSave.S
     )
-set_source_files_properties(${LIBUNWIND_ASM_SOURCES}
-                            PROPERTIES
-                              LANGUAGE C)
 
 set(LIBUNWIND_HEADERS
     AddressSpace.hpp
     assembly.h
     CompactUnwinder.hpp
     config.h
     dwarf2.h
     DwarfInstructions.hpp
     DwarfParser.hpp