This is an archive of the discontinued LLVM Phabricator instance.

[lld][ARM][NFCI][1/3]Big Endian support - Removing assumptions
ClosedPublic

Authored by simpal01 on Dec 16 2022, 3:21 AM.

Details

Summary

Change:

  • Replacing the memcpy that assume little endian with the endian-aware write.

Shouldn't affect the output for now, just a prerequisite for the next patches.

Diff Detail

Event Timeline

M-Plichta created this revision.Dec 16 2022, 3:21 AM
Herald added a project: Restricted Project. · View Herald Transcript
M-Plichta requested review of this revision.Dec 16 2022, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2022, 3:21 AM

I've noticed a couple of places where a .word is written in two 16-bit halves. This won't change the result for little-endian to little-endian writes, but could affect big-endian results later. It may also be worth adding [NFCI] to the title as this is a refactoring change (No Functional Changes Intended). Otherwise looks good to me.

I'll be on vacation for the next 3 weeks, happy for others to review in my absence. Otherwise will take another look in the new year.

lld/ELF/Thunks.cpp
664

The .word is 32-bit. This should be a write32 rather than 2 write16s.

686

The .word is 32-bit. This should be a write32 rather than 2 write16s

MaskRay accepted this revision.Dec 19 2022, 3:00 PM

Please fix The .word is 32-bit. This should be a write32 rather than 2 write16s

This revision is now accepted and ready to land.Dec 19 2022, 3:00 PM
M-Plichta marked 2 inline comments as done.Dec 20 2022, 12:59 AM
M-Plichta retitled this revision from [lld][ARM][1/3]Big Endian support - Removing assumptions to [lld][ARM][NFCI][1/3]Big Endian support - Removing assumptions.
simpal01 commandeered this revision.Feb 13 2023, 12:56 AM
simpal01 added a reviewer: M-Plichta.
simpal01 added subscribers: Milosz-Plichta, simpal01.

I am taking over this change from the original author @Milosz-Plichta with his permission to move the change forward.

simpal01 updated this revision to Diff 497355.Feb 14 2023, 9:02 AM

Rebased this patch. It was needed a bit more changes to work with the below changes:
https://reviews.llvm.org/D141272
https://reviews.llvm.org/D139888

MaskRay accepted this revision.Feb 14 2023, 9:51 AM

This can be pushed on its own, no need to wait for the follow-up.

This revision was landed with ongoing or failed builds.Feb 15 2023, 3:43 AM
This revision was automatically updated to reflect the committed changes.