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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D67656#1680324 <https://reviews.llvm.org/D67656#1680324>, @jhenderson wrote:

> Looks good to me, but as agreed, let's wait for the GNU objcopy discussion to reach a conclusion before committing. I've posted there expressing my view. FWIW, our proprietary equivalent to objcopy uses raw values for this, rather than powers of two stuff, so the behaviour proposed in this patch would make life easier for them if and when they migrate over.


+1, whichever choice we take, the two tools should be in agreement, so let's wait. Also a personal +1 for preferring setting the literal value as in this patch rather than the exponent.



================
Comment at: test/tools/llvm-objcopy/ELF/binary-input.test:119-120
+# ALIGN:      Name: .data
+# ALIGN:      AddressAlignment:
+# ALIGN-SAME: 8
----------------
I don't think splitting it into -SAME lines is very readable here; it seems more useful in cases where it needs to be a different check prefix. Combine it?


================
Comment at: test/tools/llvm-objcopy/ELF/set-section-alignment.test:7-9
+# CHECK:      Name: .foo
+# CHECK:      AddressAlignment:
+# CHECK-SAME: 4
----------------
Same for all the -SAME in this file


================
Comment at: test/tools/llvm-objcopy/ELF/set-section-alignment.test:37-39
+# RUN: not llvm-objcopy --set-section-alignment=.foo=bar %t /dev/null 2>&1 | \
+# RUN:   FileCheck --check-prefix=INVALID-ALIGN %s
+# INVALID-ALIGN:   error: invalid alignment for --set-section-alignment: 'bar'
----------------
Also add a test for a very large alignment, e.g. 2^33?


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:170-173
+  if (Split.second.getAsInteger(0, NewAlign))
+    return createStringError(errc::invalid_argument,
+                             "invalid alignment for --set-section-alignment: '%s'",
+                             Split.second.str().c_str());
----------------
What about an error check for NewAlign != 2^x?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67656