Fixes PR43181. This option was recently added to GNU objcopy (binutils
PR24942).
llvm-objcopy -I binary -O elf64-x86-64 --set-section-alignment .data=8 can set the alignment of .data.
Fixes PR43181. This option was recently added to GNU objcopy (binutils
PR24942).
llvm-objcopy -I binary -O elf64-x86-64 --set-section-alignment .data=8 can set the alignment of .data.
Buildable 38234 | |
Build 38233: arc lint + arc unit |
binutils commit fa463e9fc644e7a3bad39aa73bf6be72ea865805 (2019-08-28) implemented --set-section-alignment.
Reported both issues at https://sourceware.org/bugzilla/show_bug.cgi?id=24942
test/tools/llvm-objcopy/ELF/binary-input.test | ||
---|---|---|
120 | We sometimes use something like: # ALIGN: Name: .data # ALIGN: AddressAlignment: # ALIGN-SAME: 8 Its a bit more straightforward way probably. | |
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
28 | The number of spaces between ":" and "error" is different in these 4 tests. I'd suggest stick to a single space everywhere. btw, should this one be a error? Tools sometimes allows overriding the options (e.g. linker), it does not seem to be a huge problem to set | |
40 | What about adding a section with alignment != 0 and a test that shows it can be dropped to 0? |
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
---|---|---|
28 |
Yes, they are aligned by error: .
I agree this does not have to be an error. I'll let the last option win. |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
765 | I'm confused, you still go though handleArgs here. Why do you need to handle this flag above? |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
765 | They are not needed. handleArgs sets the alignment of the newly created .data. I don't know why I added the code. |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
673 | Is this idea behind not putting this and the SetSectionFlags loop that does the same thing in the same loop because they are unlikely to be used? Without looking very far I see 4 times including here that we go through every section. Not that it makes sense for this patch but it might eventually be worth it to hash every section by name into a StringMap so we only iterate once over all sections and can handle SetSectionAlignment, SetSectionFlags and SectionsToRename more efficiently. |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
673 | I think the time is dominated by the hash table lookup SetSectionAlignment.find, not the loop itself. So separate loop is probably better if we unlikely do lots of different operations in the same invocation of llvm-objcopy. The same strategy is used by replaceAndRemoveSections. Though, I am not there it is the best there. The updateAndRemoveSymbols way may be simpler. I'd like to explore whether regrouping some operations may make the logic clearer there. |
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.
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
---|---|---|
36 | What about a test case for where the section does not exist? | |
tools/llvm-objcopy/CopyConfig.cpp | ||
172 | Maybe "invalid alignment" would be a slightly better wording? I'm not sure though. | |
604 | It's not clear to me what SAU stands for. Perhaps NameAndAlignment would be a better name? | |
tools/llvm-objcopy/ELF/Object.h | ||
947 | This change seems unrelated? (Happy for it to be a separate commit though). |
The behavior difference is also my concern, so I hold off on this patch. I argue at https://sourceware.org/bugzilla/show_bug.cgi?id=24942#c9 that --set-section-alignment .foo=8 => sh_addralign=256 is counterintuitive. Please chime in if you feel the same.
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
604 | Changed to NameAndAlign (NameAndAlignment would cause a line wrap). | |
tools/llvm-objcopy/ELF/Object.h | ||
947 | Thanks for noting this. I originally added a new parameter here. It seems I forgot to delete the parameter when I switched to a new approach. |
When you say chime in do you mean here or on that bug report?
Yes I also think that specifying in powers of two is very strange when no other alignment related flags accept input like this. It seems like from what the people on that bug report said they went for this behavior because bfd represents section alignment in this form. Which is a silly reason to make users need to just remember that this one flag accepts alignment as 2^<align>. In their patch, I wouldn't even get an error --set-section-alignment .foo=256 would later become 1 << 256 if I followed their code correctly after a quick skim. Now my section has sh_addralign=0 with no warning why, this flag isn't matured yet on their end.
I don't know how long you will need to wait for them to change it given this was committed very recently, and they seemed uninterested in your suggestion to change it to something more sensible.
I mean https://sourceware.org/bugzilla/show_bug.cgi?id=24942 , the bugzilla ticket where people discuss the --set-section-alignment option in GNU objcopy. Can you please forward the comment there:) ?
If you'd like to play with GNU objcopy:
git clone git://sourceware.org/git/binutils-gdb.git cd binutils-gdb mkdir Debug ../configure --enable-binutils --disable-gas --disable-gdb --disable-gold --disable-ld CFLAGS='-O0 -g' make -j all-binutils # Built binutils/objcopy
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.
Looks like the author of that patch submitted a new patch to accept alignment of power of twos not 2^<align> after James’ comment :)
That patch will give an error if the given <align> is not a power of two Including 0. Do you have thoughts on erroring for non powers excluding 0?
Also, it might be worth adding a test setting alignment to a negative value to make sure StringRef::getAsInteger<unsigned> properly fails.
+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.
test/tools/llvm-objcopy/ELF/binary-input.test | ||
---|---|---|
119–120 | 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? | |
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
8–10 | Same for all the -SAME in this file | |
38–40 | Also add a test for a very large alignment, e.g. 2^33? | |
tools/llvm-objcopy/CopyConfig.cpp | ||
170–173 | What about an error check for NewAlign != 2^x? |
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
---|---|---|
8–10 | 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 | |
38–40 | 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. | |
tools/llvm-objcopy/CopyConfig.cpp | ||
170–173 | 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. |
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
---|---|---|
8–10 | +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 |
test/tools/llvm-objcopy/ELF/set-section-alignment.test | ||
---|---|---|
8–10 | 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 :) | |
38–40 | 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). | |
tools/llvm-objcopy/CopyConfig.cpp | ||
170–173 | 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. |
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
170–173 | 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:
So I think it is not necessary to restraint --set-section-alignment= to what the ELF spec allows. |
test/tools/llvm-objcopy/ELF/binary-input.test | ||
---|---|---|
120 | ALIGN-SAME: 8{{$}} maybe to avoid spuriously matching against '8000' or whatever. Same goes elsewhere. |
Thanks to everyone who has taken part in the discussion. GNU objcopy mainline has switched to the raw value instead of a power of two: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=146a3bb9cc78c459e63c23259d766b01c32df705
With a bit of luck this change may be included in the binutils 2.33 release sources https://sourceware.org/ml/binutils/2019-10/msg00011.html
I'll commit this change in a minute.