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

[Fangrui Song via Phabricator via llvm-commits]

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


================
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:
> 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.
I agree the ELF spec does not say the alignment can be a non-power-of-two. Allowing it can be seen as an extension, just like some language constructs that are accepted by gcc/clang but not allowed by the C/C++ standard. I think allowing non-power-of-two is fine:

* There are many ways to create an executable/shared object that cannot run.
* --set-section-alignment=5 can be seen as a strong signal that the user requests for an invalid setting. This can be used for testing purposes. For example, yaml2obj is capable to create lots of types of invalid objects.
* sh_addralign is ignored for executables/object files. When applied on an object, a gold or lld user will soon notice the problem if they do --set-section-alignment=5 but expected 32 => `ld.lld: error: b.o:(.baz): sh_addralign is not a power of 2`, `gold: error: b.o: invalid alignment 17 for section ".baz"`. GNU ld does not error, but silently rounded up sh_addralign. I think I shall report a bug.
* Adding the error checking takes some lines of code.

So I think it is not necessary to restraint --set-section-alignment= to what the ELF spec allows.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67656