[Patch] Let MC/ELF generate Thumb/Thumb-2 are properly

Hi,

We are trying to use clang as a drop-in replacement for the gcc come with
Android NDK. I found that MC/ELF doesn't not handle Thumb functions properly,
e.g., bit 0 of the function name in the .symtab is not set to 1, and some thumb
instructions are not generated correctly, e.g., the addresses for tBL/tBLX are
not calculated right.

With that attached patch, we can compile and run some (not all) Android
NDK samples without problem.

The attached initial patch
1. sets bit 0 of the function address of thumb function in .symtab
   ("T is 1 if the target symbol S has type STT_FUNC and the
   symbol addresses a Thumb instruction ;it is 0 otherwise."
   from "ELF for the ARM Architecture" 4.7.1.2)

2. fixes target address tBL and tBLX
3. sets relocation type of tBL/tBLX to R_ARM_THM_CALL
    (4.7.1.6)
4. adds some attributes to attribute section when cpu is "xscale"
   (this is what used in Android NDK, when architecture is ARMv5)

llvm-thumb-elf-hack.diff3 (5.88 KB)

Hi Koan,

Have you tried to run Intel and other platform tests?

I feel uncomfortable with so many changes in generic MC/ELF regarding
specific ARM support... not to mention the magic variables on
shifts... :wink:

cheers,
--renato

Hi Renato,

  Nope, I didn't run other platform tests. I'll do it. Thanks :slight_smile:
  The reason of doing ARM specific thing in generic MC/ELF
is that I don't know platform specific place to make changes.

Renato,

  The result of running 'llvm-lit $LLVM/TEST/MC" is:
Expected Passes : 330
Expected Failures : 20
Actually it passed "make check", no unexpected result.

Try make check-all, it runs more tests...

Still, I think someone more in line with MC/ELF should have a look at
the patch. Rafael? Jason?

cheers,
--renato

  The result of running 'llvm-lit $LLVM/TEST/MC" is:
Expected Passes : 330
Expected Failures : 20
  Actually it passed "make check", no unexpected result.

Try make check-all, it runs more tests...

Still, I think someone more in line with MC/ELF should have a look at
the patch. Rafael? Jason?

When we first started adding arm we decided to make it work first and refactor it afterwards since none of us was sure how much sharing there could be. Given that, it is inevitable that some changes will be to the common code for now.

The patch doesn't violate any of the designs decisions of MC, so I am OK with it if you add a test. Ideally it should be a .s -> .o test, but if you need to test something that is not yet handled by the ARM parser, then it is ok to have a .ll -> .o test, but add a FIXME to it.

cheers,
--renato

Cheers,
Rafael

Hi Koan,

In general, this looks OK to me. Please split the patch into separate pieces, one for each issue you're addressing, though. From your description, it sounds like this should be 4 patches. That way we have a cleaner revision history in svn.

-Jim

Jim,

Thanks, I'll do it accordingly.

Hi Rafael,

Thanks for the explanation on the situation of ARM ELF.

I'll learn to write .s -> .o tests.

splited patches and test cases

mc-elf-thumbfunction-bit.diff: for 1
mc-elf-arm-backend-bl-blx-sign-bit.diff: for 2.
mc-elf-thumb-bl-blx-relocation-table-entry.diff: for 3.
mc-elf-cpu-xscale-attributes.diff: for 4.

elf-thumbfunc.s: test case for 1
elf-thumbfunc-reloc.ll: test case for 2 and 3
elf-xscale-attribute.ll: test case for 4

mcelf-thumb.tgz (3.7 KB)

I have checked in the first patch and test (with small modifications to the test).

For the next patches, can you include the tests in the patch itself? If using svn you can just svn add the test before running svn diff.

Can elf-thumbfunc-reloc.ll be converted to a llvm-mc (.s -> .o) test? That would make it a lot more resistant to changes in codegen.

Same for elf-xscale-attribute.ll (or maybe this one should be a .ll -> .s?).

Thanks,
Rafael

Thanks for the review and checkin.

Regarding elf-thumbfunc-reloc.ll, it seems to me that current ARMAsmParser
doesn't recognize "(PLT)", so something like "bl foo(PLT)" doesn't work
consequently. And I don't know how to write .s to test this without (PLT).

Regarding elf-xscale-attribute.ll, those attributes are for
".eabi_attribute" lines
in .s files and attributes in .aeabi section in .o files. Both .ll->.s
and .ll->.o
should be tested. Surely, not .s -> .o. I didn't write .ll -> .s because
ARMAsmParser ignore ".eabi_attribute" for now

Thanks for the review and checkin.

Thanks for the patch!

Regarding elf-thumbfunc-reloc.ll, it seems to me that current ARMAsmParser
doesn't recognize "(PLT)", so something like "bl foo(PLT)" doesn't work
consequently. And I don't know how to write .s to test this without (PLT).

NP. Can you just add that as a FIXME in elf-thumbfunc-reloc.ll? That way we know why it is there and can port it once more of the asm parser works.

Regarding elf-xscale-attribute.ll, those attributes are for
".eabi_attribute" lines
in .s files and attributes in .aeabi section in .o files. Both .ll->.s
and .ll->.o
should be tested. Surely, not .s -> .o. I didn't write .ll -> .s because
ARMAsmParser ignore ".eabi_attribute" for now

Since this is something that happens in Codegen (.ll -> .s), I think this should probably be tested by looking at the .s produced. The fact that llvm-mc is still not able to process .eabi_attribute is a different issues which should get its own tests once it is able to.

Thanks,
Rafael

Regarding elf-thumbfunc-reloc.ll, it seems to me that current ARMAsmParser
doesn't recognize "(PLT)", so something like "bl foo(PLT)" doesn't work
consequently. And I don't know how to write .s to test this without (PLT).

NP. Can you just add that as a FIXME in elf-thumbfunc-reloc.ll? That way we
know why it is there and can port it once more of the asm parser works.

Done!

Regarding elf-xscale-attribute.ll, those attributes are for
".eabi_attribute" lines
in .s files and attributes in .aeabi section in .o files. Both .ll->.s
and .ll->.o
should be tested. Surely, not .s -> .o. I didn't write .ll -> .s
because
ARMAsmParser ignore ".eabi_attribute" for now

Since this is something that happens in Codegen (.ll -> .s), I think this
should probably be tested by looking at the .s produced. The fact that
llvm-mc is still not able to process .eabi_attribute is a different issues
which should get its own tests once it is able to.

.ll -> .s test added. As far as I can tell .ll -> .s and .ll -> .o go
different paths, so I have both .ll->.s and .ll -> .o in the test case

The revised patches (with tests in .diff) are attached.

patch-2-3.diff:
  fixes target address tBL and tBLX and sets relocation type
  of tBL/tBLX to R_ARM_THM_CALL (ARM ELF 4.7.1.6)

patch-4.diff:
  adds some attributes to attribute section when cpu is "xscale"
  (this is what used in Android NDK, when architecture is ARMv5)

Thanks,
// koan-sin tan

mc-elf-thumb.tgz (1.93 KB)

.ll -> .s test added. As far as I can tell .ll -> .s and .ll -> .o go
different paths, so I have both .ll->.s and .ll -> .o in the test case

The revised patches (with tests in .diff) are attached.

patch-2-3.diff:
   fixes target address tBL and tBLX and sets relocation type
   of tBL/tBLX to R_ARM_THM_CALL (ARM ELF 4.7.1.6)

patch-4.diff:
   adds some attributes to attribute section when cpu is "xscale"
   (this is what used in Android NDK, when architecture is ARMv5)

Done. Just two nits: A single ';' is sufficient to start a comment and it is easier to review patches if you attach them directly to the email.

Thanks,
// koan-sin tan

Thanks,
Rafael