Page MenuHomePhabricator

[AArch64] Replace some uses of report_fatal_error with reportError in AArch64 ELF object writer
ClosedPublic

Authored by olista01 on Mar 23 2016, 3:20 AM.

Details

Summary

If we can't handle a relocation type, report it as an error in the source, rather than asserting. I've added a more descriptive message and a test for the only case of this that I've been able to trigger.

Diff Detail

Repository
rL LLVM

Event Timeline

olista01 updated this revision to Diff 51394.Mar 23 2016, 3:20 AM
olista01 retitled this revision from to [AArch64] Replace some uses of report_fatal_error with reportError in AArch64 ELF object writer.
olista01 updated this object.
olista01 added a reviewer: t.p.northover.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: llvm-commits.

Hi Oliver,

I think this is a great idea, but there are too many new error messages and too few new tests. Can you add a test each?

cheers,
--renato

I haven't been able to trigger any of the others, as they are checked when the fixups are created (and we don't have the .reloc directive on AArch64 to create arbitrary fixups). The .byte directive is the only way I know of to trigger this at the moment.

Have you tried creating a few instructions with invalid ranges? I understand that this may break other things in the back-end before it gets to that part.

But following the adr example, you may find a way to break movz/movk or at least some load/store ones.

olista01 updated this revision to Diff 51401.Mar 23 2016, 5:40 AM
olista01 removed rL LLVM as the repository for this revision.

The ADR and MOVK/Z fixups are quite accurately checked during operand parsing, but I have managed to find a few cases where invalid ldr/str immediates get through to here.

This function only checks the type of relocation, not the range. I've found some cases where out-of-range relocations hit report_fatal_error in adjustFixupValue, but that's a separate issue (that I'll look at next).

rengolin accepted this revision.Mar 23 2016, 6:29 AM
rengolin added a reviewer: rengolin.

Makes sense. LGTM. Thanks!

This revision is now accepted and ready to land.Mar 23 2016, 6:29 AM
This revision was automatically updated to reflect the committed changes.