This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Add ifdefs around ELF specific parts of UnwindRegisters*.S for ARM
ClosedPublic

Authored by mstorsjo on Nov 2 2017, 1:30 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 2 2017, 1:30 AM
compnerd requested changes to this revision.Nov 2 2017, 1:33 PM
compnerd added inline comments.
src/UnwindRegistersRestore.S
396

This really isn't an ELF vs COFF/Mach-O thing. Does MASM let you do something similar? This should at the very least be !defined(_WIN32). The same throughout.

This revision now requires changes to proceed.Nov 2 2017, 1:33 PM
mstorsjo added inline comments.Nov 2 2017, 1:40 PM
src/UnwindRegistersRestore.S
396

MASM is only for x86, and MS armasm uses a wholly different syntax (with afaik nothing similar to .arch or .fpu).

And similarly, if you try assembling a file with this directive, it will fail both for iOS and windows:

$ cat test.s
.text
.fpu vfpv3-d16
add r0, r0, r0
$ clang -target armv7-linux-gnueabi test.s -c -o test.o
$ clang -target armv7-apple-darwin test.s -c -o test.o
test.s:2:1: error: unknown directive
.fpu vfpv3-d16
^
$ clang -target thumbv7-win32-gnu test.s -c -o test.o
test.s:2:1: error: unknown directive
.fpu vfpv3-d16
^
mstorsjo added inline comments.Nov 3 2017, 12:23 AM
src/UnwindRegistersRestore.S
396

If you really wanted to, I could make it !defined(__APPLE__) && !defined(_WIN32), but I really think defined(__ELF__) is more straightforward. (The corresponding parts in the asm parser in LLVM is within an if (!IsMachO && !IsCOFF) block though.)

compnerd accepted this revision.Nov 4 2017, 1:41 PM

Very well, if thats the current implementation in the AsmParser, thats reasonable. I don't think that the directive has anything to do with the file format though.

This revision is now accepted and ready to land.Nov 4 2017, 1:41 PM

Very well, if thats the current implementation in the AsmParser, thats reasonable. I don't think that the directive has anything to do with the file format though.

I can agree with that. In addition to making the assembler accept/reject certain instructions though, it actually does another thing which actually is file format specific - it sets the eabi attributes that indicates that the object file contains such instructions.

This revision was automatically updated to reflect the committed changes.

Very well, if thats the current implementation in the AsmParser, thats reasonable. I don't think that the directive has anything to do with the file format though.

I can agree with that. In addition to making the assembler accept/reject certain instructions though, it actually does another thing which actually is file format specific - it sets the eabi attributes that indicates that the object file contains such instructions.

Are they eabi/gnueabi things?

Very well, if thats the current implementation in the AsmParser, thats reasonable. I don't think that the directive has anything to do with the file format though.

I can agree with that. In addition to making the assembler accept/reject certain instructions though, it actually does another thing which actually is file format specific - it sets the eabi attributes that indicates that the object file contains such instructions.

Are they eabi/gnueabi things?

I think so. You can read them with readelf -A foo.o, and override them with manual .eabi_attribute attributes. (That's useful e.g. for indicating that while a binary contains NEON instructions, it doesn't strict require them for running. E.g. raspbian does check such tags for checking that all binaries work on their baseline of armv6.)

Very well, if thats the current implementation in the AsmParser, thats reasonable. I don't think that the directive has anything to do with the file format though.

I can agree with that. In addition to making the assembler accept/reject certain instructions though, it actually does another thing which actually is file format specific - it sets the eabi attributes that indicates that the object file contains such instructions.

Are they eabi/gnueabi things?

I think so. You can read them with readelf -A foo.o, and override them with manual .eabi_attribute attributes. (That's useful e.g. for indicating that while a binary contains NEON instructions, it doesn't strict require them for running. E.g. raspbian does check such tags for checking that all binaries work on their baseline of armv6.)

i.e. should this be keyed off of __ARM_EABI__ instead?

i.e. should this be keyed off of __ARM_EABI__ instead?

I'm not really sure. As I quoted earlier, the exact case for enabling these directives in the LLVM codebase right now is (!IsMachO && !IsCOFF).

We could make this !defined(__MACH__) && !defined(_WIN32), but I felt that defined(__ELF__) was more readable.

I'm not familiar enough with the other ARM configurations to know what makes most sense for them. Which ones might I be missing here - non-EABI ELF configurations, or non-ELF EABI configurations, or both? If it's non-EABI ELF configurations, the current directive should be fine I think.