Broken PLT on ARM from R183966

For ARM targets on linux, revision 183966 made Fast ISel default. Unfortunately, Fast ISel is broken in terms of applying the ARMII::MO_PLT flags to calls in PIC mode (at least when emitting assembly); it never does this. The normal ISel pass handles this situation correctly so a temporary local change to disable FastISel for linux / NaCl targets is working for me right now.

I’m not very familiar with the ISel passes. I’m guessing the correct thing to do here would be to apply the attribute correctly in FastISel so it works, but I’m kind of unfamiliar with this part of the code and won’t have time to dig into it right now, although I’m willing to do it if someone points me to the right area of code.

In the meantime, would it be worthwhile to submit in a modification to disable FastISel for non-darwin targets, or is it likely to be fixed quickly?

-Gordon

Filing a bug would be a good start, go ahead and cc me and jfb@google.com.

Thanks!

-eric

Cool, I'll file a bug tomorrow at work and add you to the CC list.

Thanks!
Gordon Keiser
Software Development Engineer
Arxan Technologies
gkeiser@arxan.com www.arxan.com
Protecting the App EconomyT

Hi,

I have created a workaround to deal with the PIC function call. With this patch, the FastISel will switch back to DAG lowering mechanism if (1) there is a function call in the basic block and (2) the relocation model is PIC. Please have a look. Hoping the patch will help.

Sincerely,
Logan

0001-Workaround-for-ARM-FastISel-PIC-function-call.patch (1.68 KB)

LGTM

lgtm

Hi Anton and JF,

Thanks for your review. After reading the source code more carefully, I have come up with a different way fix this issue. We can simply resolve this issue by adding ARMII::MO_PLT flags with MachineInstrBuilder in FastISel pass (without failing back to DAG lowering).

The new patch is attached, and the test case is not changed. Sorry for your inconvenience. Please have a look. Thanks for your help.

Sincerely,
Logan

0001-Fix-ARM-FastISel-PIC-function-call.patch (2.28 KB)

I’m not very familiar with relocations but your fix looks the same as ARMTargetLowering::LowerCall, so from that perspective it lgtm (but I may be missing something).

That change seems to fix things here. Thanks!

-Gordon

Thanks. Committed as r189002.

Logan