This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Rename relocateOne() to relocate() and pass `Relocation` to it
ClosedPublic

Authored by MaskRay on Jan 22 2020, 10:39 PM.

Details

Summary

Symbol information can be used to improve out-of-range/misalignment diagnostics.
It also helps R_ARM_CALL/R_ARM_THM_CALL which has different behaviors with different symbol types.

There are many (67) relocateOne() call sites used in thunks, {Arm,AArch64}errata, PLT, etc.
Rename them to relocateNoSym() to be clearer that there is no symbol information.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 22 2020, 10:39 PM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

grimar added inline comments.Jan 23 2020, 12:55 AM
lld/ELF/Arch/PPC64.cpp
790

In terms of the original code, seems previously this line used a new type, but now it uses an originalType.
Is it intentional and correct change?

885

The same here and below.

lld/ELF/Target.h
90

It is probably looks a bit strange to have such piece here.
It does not look to be generic code because initializes only single field.
And it seems used only once.

MaskRay marked 5 inline comments as done.Jan 23 2020, 9:12 AM
MaskRay added inline comments.
lld/ELF/Arch/PPC64.cpp
790

It is intentional.

type is only used in a diagnostic: Target.h:reportRangeError.

The code handle several similar relocation types together, but the original code made a mistake (type can be modified but the diagnostic should report the original relocation type). So this change actually also fixes a diagnostic bug (not serious, and I don't expect such alignment errors to happen in practice)

885

They fixed the bug.

lld/ELF/Target.h
90

relocateOne is very common.

% rg relocateOne lld | wc -l
72

The intention is to thread Symbol through relocation handling (context: https://github.com/ClangBuiltLinux/linux/issues/773)

For many relocateOne, there is probably not a good Symbol * to thread through. We can use nullptr, but that will requires changes in the call sites. I think it is just simpler to keep the utility here.

peter.smith added inline comments.Jan 23 2020, 9:13 AM
lld/ELF/Target.h
90

I think that there are quite a few callsites that are in files unaffected by this diff or in the context, for example Thunks, PLT sequences. As I understand it they are a way of using the relocateOne logic to avoid duplicating some potentially nasty encoding logic.

This does raise an interesting question though as we don't always have a symbol when doing such a relocation. For example we can make a Relocation, like relocateOne above but we may not be able to fake a symbol in all contexts.

This would mean that this patch would work for rel->type as there would always be a type, but we'd have to be careful about testing that a symbol exists before using it.

MaskRay marked 2 inline comments as done.Jan 23 2020, 9:30 AM

Thanks for the patch!

lld/ELF/Target.h
91

It looks like there's just a few more callers of relocateOne in:

  • ELF/AArch64ErrataFix.cpp
  • ELF/ARMErrataFix.cpp
  • ELF/InputSection.cpp

I think if you updated those too, you could drop this change to relocateOne and then this patch would be simply renaming relocateOne to relocate and taking a full Relocation object rather than a RelType.

You might even give struct Relocation an non-explicit constructor that way you could implicitly construct a Relocation via a RelType. Then calls to relocateOne with a literal RelType would only have to be renamed, and you wouldn't have to add code to contruct Relocations at callsites.

(It looks like struct Relocation in llvm/tools/llvm-objcopy/COFF/Object.h already does something to this effect, for prior art). Oh man, so many different Relocation structs across the codebase...

246

Are the changes to this file necessary? Is the full Relocation object used?

MaskRay marked an inline comment as done.Jan 23 2020, 9:38 AM
MaskRay added inline comments.
lld/ELF/Target.h
91

I think if you updated those too, you could drop this change to relocateOne and then this patch would be simply renaming relocateOne to relocate and taking a full Relocation object rather than a RelType.

As @peter.smith explained above, " there are quite a few callsites that are in files unaffected by this diff or in the context, for example Thunks, PLT sequences".

We can probably pass Symbol * to relocateOne() in Thunks.cpp, but some call sites like writePltHeader() there is no meaningful Symbol *, so we will have to use nullptr.

An alternative signature is:

void relocate(uint8_t *loc, RelType type, uint64_t val, const Symbol *sym = nullptr) const;

I considered it but still preferred the current one:

void relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const;

because the signature will be consistent with relaxGot() and relaxTls* (https://reviews.llvm.org/D73250). We do need to be careful and not use Relocation members other than type and sym.

Looking at where we use relocateOne directly, I can see the PLT code, ARMErrataFix and AArch64ErrataFix , and Thunks that call relocateOne directly. For my selfish purposes this isn't a problem as for correcting ARM/Thumb interworking via BLX I'd only need to know the symbol type for R_ARM_CALL and R_ARM_THM_CALL which always have a symbol. I think that the R_ARM_THM_JUMP19, R_ARM_THM_JUMP24 and R_ARM_JUMP24 can be solved in needsThunk() which already has the information it needs.

I'm a bit torn about the interface as there are trade-offs. Having

void relocate(uint8_t *loc, RelType type, uint64_t val, const Symbol *sym = nullptr) const;

Makes it a bit more explicit that sym can be nullptr. Perhaps instead of relocate and relocateOne we rename relocateOne to relocateWithoutSym(), a bit more verbose but there aren't too many callsites?

MaskRay updated this revision to Diff 239931.Jan 23 2020, 10:07 AM
MaskRay edited the summary of this revision. (Show Details)

Rename remaining relocateOne() call sites to relocateWithoutSym()

MaskRay marked 3 inline comments as done.Jan 23 2020, 10:18 AM
MaskRay added inline comments.
lld/ELF/Symbols.cpp
102

@atanasyan Looks like the relocateOne(..., RelType type, ...) -> relocate(..., const Relocation &rel, ...)` change (this patch) may benefit MIPS as well?

lld/ELF/Target.h
246

This is for a future change: adding Symbol * information to the diagnostic.

MaskRay updated this revision to Diff 239935.Jan 23 2020, 10:21 AM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)

Convert 5 relocateWithoutSym call sites to use relocate()

(There is no functional change, but it can improve diagnostics in the future.)

Thanks for the update, will be worth seeing what George thinks tomorrow.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 62139 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 108 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

grimar added inline comments.Jan 24 2020, 1:21 AM
lld/ELF/AArch64ErrataFix.cpp
424

Is relocateNoSym sounds better?

lld/ELF/Target.h
88

This creates an R_ABS relocation, what is probably not ideal.

What do you think about the following?

void relocateWithoutSym(uint8_t *loc, RelType type, uint64_t val) const {
  relocate(loc, {R_NONE, type, 0, 0, nullptr}, val);
}
peter.smith added inline comments.Jan 24 2020, 2:40 AM
lld/ELF/AArch64ErrataFix.cpp
424

I'm happy with either, relocateNoSym is a bit shorter.

lld/ELF/Target.h
88

I agree, that R_NONE is better, even if we're unlikely to use it. As an alternative we could move R_NONE to the top of RelExpr and set it explicitly to 0, but I'm happy with the suggestion here.

MaskRay updated this revision to Diff 240221.Jan 24 2020, 9:03 AM
MaskRay edited the summary of this revision. (Show Details)

relocateWithoutSym -> relocateNoSym
Set expr to R_NONE

MaskRay marked 6 inline comments as done.Jan 24 2020, 9:03 AM
MaskRay updated this revision to Diff 240222.Jan 24 2020, 9:04 AM

clang-format

Unit tests: fail. 62172 tests passed, 5 failed and 815 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 108 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 62170 tests passed, 7 failed and 815 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_recursive/try_lock_for.pass.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_recursive/try_lock_until.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 108 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

peter.smith accepted this revision.Jan 24 2020, 10:35 AM

Thanks for the update, I'm happy with the changes, will be worth waiting for George's ack as well.

This revision is now accepted and ready to land.Jan 24 2020, 10:35 AM
grimar added inline comments.Jan 25 2020, 9:34 AM
lld/ELF/Target.h
92
Relocation rel{};
rel.expr = R_NONE;
rel.type = type;

This will initialize the expr and type fields twice, isn't?
First to the default values and then to R_NONE and type.
Applying relocations is a path that is performance sensible.
I am not sure how compiler can/will optimize this place, but why don't you just want to use
relocate(loc, {R_NONE, type, 0, 0, nullptr}, val); to avoid double initialization?

MaskRay marked an inline comment as done.Jan 25 2020, 9:53 AM
MaskRay added inline comments.
lld/ELF/Target.h
92

The compiler can easily detect that the duplicate initialization can be optimized out. relocateNoSym call sites are not sensitive (pltHeader,thunk,errata,... ; these are not frequently called).

{R_NONE, type, 0, 0, nullptr} => some members are of the same type. If we made a mistake reordering 2 members, it might not be easy to detect.

The explicit approach does not look very bad.

grimar accepted this revision.Jan 25 2020, 11:24 AM

Ok, LGTM.

lld/ELF/Target.h
92

The explicit approach does not look very bad.

Looks ugly to me :)

MaskRay updated this revision to Diff 240392.Jan 25 2020, 11:57 AM

Adopt grimar's suggestion

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62195 tests passed, 0 failed and 815 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 107 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.