Page MenuHomePhabricator

[AArch64] Attempt to parse expressions as adr/adrp operands
ClosedPublic

Authored by dmgreen on Sep 7 2018, 8:26 AM.

Details

Summary

This tries to make use of evaluateAsRelocatable in AArch64AsmParser::classifySymbolRef to
parse more complex expressions as relocatable operands to ADR/ADRP's. It is hopefully
better than the existing code which only handles Symbol +- Constant.

Fixes PR38831

Diff Detail

Repository
rL LLVM

Event Timeline

dmgreen created this revision.Sep 7 2018, 8:26 AM
efriedma added inline comments.Sep 7 2018, 12:35 PM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
5289 ↗(On Diff #164431)

classifySymbolRef is used for other instructions, including movw/movk, add/sub, and ldr. Please make sure we have appropriate test coverage.

It looks like a lot of instructions try to do some sort of fallback if classifySymbolRef fails. I'm not sure that actually make sense, but please make sure we have test coverage, at least.

5313 ↗(On Diff #164431)

It looks like there isn't any test coverage for the !Res.getSymA() || Res.getSymB() part? In particular, I'm not sure why you'd want to reject an absolute value, in general.

There should probably also be test coverage that someone doesn't write something silly like adr x0, :lo12:f.

test/MC/AArch64/adr-badsyms.s
36 ↗(On Diff #164431)

Newline?

cc @mcgrathr @phosek

Awesome! I'm looking at this now.

I don't have much to add over Eli's existing comments. I think the general form of (symA - symB) + constant may not be handled well for all instructions though.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
5313 ↗(On Diff #164431)

In principle I agree that something like

 .text
lab1: nop
lab2: nop
 adr x0, lab2 - lab1 + 0x1000

which would be rejected by the code above, but is equivalent to adr x0, 0x1004 which is.

From a brief experiment I don't think that the backend can handle expressions like that for all relocations though. I ended up with a RELA relocation with addend 0x1004 to an undefined symbol, which unsurprisingly didn't get past the linker, we'd need an absolute symbol with value 0. May be if the backend were fixed first we could let that one through.

dmgreen updated this revision to Diff 165473.Sep 14 2018, 5:32 AM

I seem to have stepped into a bit of a rabbit hole here.. and a lot of things seem to be a bit broken.

As far as I understand, something like (lab2-lab1)*2 + Sym should be accepted (if the difference between lab2 and lab1 is know, Sym can be relocated against). This is handled by gcc, but I don't see how it would be easily handled by clang atm. We are trying to classify these operands quite early.

I haven't managed to fix that here, but have added a number of tests around the place for things I couldn't see otherwise tested. Let me know if I should add any more.

I've also made mov's only accept 0 addends and ld/st accept addends > 0 and not mod scale. I think this is OK (it seems to be what gcc does, you don't know the alignment of Sym afterall), but I'm not sure if it needs to work that way for darwin.

efriedma accepted this revision.Sep 17 2018, 1:13 PM

LGTM, but please give Peter a couple days to see if he has any additional comments.

This revision is now accepted and ready to land.Sep 17 2018, 1:13 PM

Yes I'm happy with this too.

I tested this change with Fuchsia and it's working for us. Thank you for looking into this! I'd like to see this landed as soon as possible as it unblocks the toolchain roll for us.

OK. Thanks guys.

This revision was automatically updated to reflect the committed changes.