[PATCH] D67656: [llvm-objcopy] Add --set-section-alignment

[Fangrui Song via Phabricator via llvm-commits]

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