[PATCH] D67656: [llvm-objcopy] Add --set-section-alignment
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 23 20:35:20 PDT 2019
MaskRay marked 6 inline comments as done and an inline comment as not done.
MaskRay added a comment.
In D67656#1679119 <https://reviews.llvm.org/D67656#1679119>, @jhenderson wrote:
> I'm very happy to see this change, but I hesitate to diverge in behaviour from GNU's behaviour. I certainly prefer --set-section-alginment to set it to an arbitrary alignment though, so I think we need to lean on the GNU maintainers to change their end.
The behavior difference is also my concern, so I hold off on this patch. I argue at https://sourceware.org/bugzilla/show_bug.cgi?id=24942#c9 that --set-section-alignment .foo=8 => sh_addralign=256 is counterintuitive. Please chime in if you feel the same.
================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:604
+ for (auto Arg : InputArgs.filtered(OBJCOPY_set_section_alignment)) {
+ Expected<std::pair<StringRef, uint64_t>> SAU =
+ parseSetSectionAlignment(Arg->getValue());
----------------
jhenderson wrote:
> It's not clear to me what SAU stands for. Perhaps `NameAndAlignment` would be a better name?
Changed to NameAndAlign (`NameAndAlignment` would cause a line wrap).
================
Comment at: tools/llvm-objcopy/ELF/Object.h:947
public:
- BinaryReader(MemoryBuffer *MB, const uint8_t NewSymbolVisibility)
+ BinaryReader(MemoryBuffer *MB, uint8_t NewSymbolVisibility)
: MemBuf(MB), NewSymbolVisibility(NewSymbolVisibility) {}
----------------
jhenderson wrote:
> This change seems unrelated? (Happy for it to be a separate commit though).
Thanks for noting this. I originally added a new parameter here. It seems I forgot to delete the parameter when I switched to a new approach.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67656/new/
https://reviews.llvm.org/D67656
More information about the llvm-commits
mailing list