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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 6 inline comments as done.
MaskRay added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/set-section-alignment.test:7-9
+# CHECK:      Name: .foo
+# CHECK:      AddressAlignment:
+# CHECK-SAME: 4
----------------
rupprecht wrote:
> Same for all the -SAME in this file
If I do not use `-SAME`, I'll have to enumerate all fields between Name and AddressAlignment, with `-NEXT:`. I think `-SAME` is clearer.

`CHECK: AddressAlignment: 4` can match something like:

```
Name: foo
AddressAlignment: 8

Name: bar
AddressAlignment: 4
```


================
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'
----------------
rupprecht wrote:
> Also add a test for a very large alignment, e.g. 2^33?
Huge alignment like `--set-section-alignment=.foo=0x100000001` cannot be used in portable tests. The llvm-objcopy segment/section writer will advance file offsets to a multiple of the alignment, which will create a huge sparse file. If the underlying file system does not support sparse files, we may get ENOSPC.

(As an missing optimization, we can change the layout algorithm to not advance file offsets for the next section of SHT_NOBITS (GNU objcopy has such a rule), but I don't think that is worthwhile: we would lose the nice property that sh_offset is monotonically non-decreasing)

Given the file size problem, I do not want to test the 2^33 case.


================
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());
----------------
rupprecht wrote:
> What about an error check for NewAlign != 2^x?
`set-section-alignment.test` tests non-power-of-2 alignments, e.g.

```
# RUN: llvm-objcopy --set-section-alignment .foo=4 --set-section-alignment .bar=0x5 \
# RUN:   --set-section-alignment .baz=0 %t %t.2
```

I don't think that makes sense in practice, but supporting that does not hurt anything, and saves us some error checking code.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67656