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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/set-section-alignment.test:7-9
+# CHECK:      Name: .foo
+# CHECK:      AddressAlignment:
+# CHECK-SAME: 4
----------------
jhenderson wrote:
> MaskRay wrote:
> > 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
> > ```
> +1 to keeping `-SAME`. I find it less readable if I have to read lots of irrelevant fields. Maybe indenting the number might make it more readable, but I'm not sure:
> 
> ```
> # CHECK:      Name: .foo
> # CHECK:      AddressAlignment:
> # CHECK-SAME:                   4
> ```
Ah, I see why this simplifies it now. I wasn't familiar with this FileCheck idiom -- maybe we should add this to the docs. I'll see if I can write up my understanding.

Now that I get it, there's a whole lot of other tests I think we can clean up :)


================
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'
----------------
MaskRay wrote:
> 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.
SGTM, can you just check manually that it works? I kinda want to just make sure that the `getAsInteger` method you're using won't silently fail for larger than int32 types.

If you had error checking for pow-2 types, you could also cheat by testing the error string since that would fail fast & not actually start to lay anything out in a file system (and hit the portability problems).


================
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());
----------------
MaskRay wrote:
> 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.
It would silently generate an invalid ELF file at least, per http://www.sco.com/developers/gabi/latest/ch4.sheader.html#section_header: "... Currently, only 0 and positive integral powers of two are allowed."

I think it's useful because, while it seems we all agree it's more natural to have this be the raw value, there may be some that think this is the exponent, and may expect in your example that `.bar` has an alignment of 2^5.

I don't know that Mach-O/COFF have similar restrictions, so I don't know if this check can be general or needs to go in an ELF-specific error checker.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67656