Page MenuHomePhabricator

[lld-macho] Add some relocation validation logic
ClosedPublic

Authored by int3 on Fri, May 15, 4:29 PM.

Details

Summary

I considered making a Target::validate() method, but I wasn't sure how I felt about the overhead of doing yet another switch-dispatch on the relocation type, so I put the validation in relocateOne instead... might be a bit of a micro-optimization, but relocateOne does assume certain things about the relocations it gets, and this error handling makes that explicit, so it's not a totally unreasonable code organization.

Depends on D80048.

Diff Detail

Event Timeline

int3 created this revision.Fri, May 15, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 15, 4:29 PM
int3 edited the summary of this revision. (Show Details)Fri, May 15, 4:31 PM

I'm torn about this one.

One the one hand, as you said, putting the validation in relocateOne is justifiable, since it's making a bunch of assumptions about various relocations. On the other, it feels a bit iffy to be mixing concerns like that, and it also means we won't get errors from broken input files until we start the writing process, which is pretty late.

Would it be possible to do the validation in getImplicitAddend instead? That's at least earlier. It also feels a bit better conceptually ... we could have getImplicitAddend be the function that does any target-specific relocation processing and validation. Unfortunately, I can't think of a good name that conveys that it also computes and returns the implicit addend :/

lld/test/MachO/invalid/invalid-relocation.yaml
44

CC @alexshap the support you added is coming in handy :)

int3 updated this revision to Diff 265370.Wed, May 20, 3:51 PM

move to getImplicitAddend

smeenai added inline comments.Thu, May 21, 4:51 AM
lld/MachO/Arch/X86_64.cpp
28–29

Low-priority follow-up: be consistent about including or not including parameter names in all these declarations, both here and in the header.

45–46

Nit: I feel like "invalid relocation of type X" would read better than "invalid type X relocation", both here and in getImplicitAddend below.

It would be really nice to be able to print the enum as a string instead of just its number (e.g. saying "relocation of type X86_64_RELOC_BRANCH" instead of "relocation of type 2"). There's MachOObjectFile::getRelocationTypeName for this purpose; it's a member function, but we could factor its main logic out into a static function. That definitely shouldn't block this change from going in though; it can easily just be another (low-pri) follow-up.

smeenai accepted this revision.Thu, May 21, 4:52 AM

Oops, forgot to LGTM. Thanks for the move!

This revision is now accepted and ready to land.Thu, May 21, 4:52 AM
int3 marked an inline comment as done.Thu, May 21, 12:11 PM
int3 added inline comments.
lld/MachO/Arch/X86_64.cpp
28–29

my approach has been to omit parameter names when the type is sufficiently descriptive -- which is why the names are present mostly only for parameters with primitive types

int3 marked an inline comment as done.Thu, May 21, 1:26 PM
int3 added inline comments.
lld/MachO/Arch/X86_64.cpp
45–46

sounds good. (Also on second thought I don't think we need to put invalid relocation of type X in the prefix; the specific checks will likely always depend on the type and so will mention it in the second part of the error message)

int3 updated this revision to Diff 265586.Thu, May 21, 1:28 PM

address comment

smeenai added inline comments.Thu, May 21, 1:36 PM
lld/MachO/Arch/X86_64.cpp
45–46

Looks great!

This revision was automatically updated to reflect the committed changes.