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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: test/tools/llvm-objcopy/ELF/set-section-alignment.test:35
+# RUN:   FileCheck --check-prefix=BAD-ALIGN %s
+# BAD-ALIGN:       error: bad alignment for --set-section-alignment: 'bar'
+
----------------
What about a test case for where the section does not exist?


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:172
+    return createStringError(errc::invalid_argument,
+                             "bad alignment for --set-section-alignment: '%s'",
+                             Split.second.str().c_str());
----------------
Maybe "invalid alignment" would be a slightly better wording? I'm not sure though.


================
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());
----------------
It's not clear to me what SAU stands for. Perhaps `NameAndAlignment` would be a better name?


================
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) {}
----------------
This change seems unrelated? (Happy for it to be a separate commit though).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67656/new/

https://reviews.llvm.org/D67656