This is an archive of the discontinued LLVM Phabricator instance.

COFF: Layout sections in the same order as link.exe
ClosedPublic

Authored by hans on Apr 4 2018, 8:13 AM.

Details

Summary

One place where this seems to matter is to make sure the .rsrc section comes after .text. The Win32 UpdateResource() function can change the contents of .rsrc. It will move the sections that come after, but if .text gets moved, the entry point header will not get updated and the executable breaks. This was found by a test in Chromium.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

hans created this revision.Apr 4 2018, 8:13 AM
thakis accepted this revision.Apr 4 2018, 8:18 AM

lgtm, but I don't know this code super well, so wait for ruiu too.

This revision is now accepted and ready to land.Apr 4 2018, 8:18 AM
pcc added a subscriber: pcc.Apr 4 2018, 8:42 AM
pcc added inline comments.
COFF/Writer.cpp
678

What if I have a .rsrc section as well as a section whose name does not appear in the list above? It seems like this could still lay out .rsrc before my section, which would break things.

To me it seems like the correct fix would be to just move .rsrc to the end.

pcc added inline comments.Apr 4 2018, 10:54 AM
COFF/Writer.cpp
678

That is what D45273 does.

ruiu added inline comments.Apr 4 2018, 11:14 AM
COFF/Writer.cpp
648–649

std::vector<StringRef> is perhaps better.

651

i -> I
e -> E

678

I think I agree with Peter about ".rsrc" section ordering, but I believe sorting all output sections in the same order as MSVC does is beneficial if there's no reason to keep the current section ordering. Changing section ordering shouldn't affect anything, but in reality it should reduce "surprise" like this.

hans updated this revision to Diff 141004.Apr 4 2018, 11:15 AM
hans edited the summary of this revision. (Show Details)
hans added a comment.Apr 4 2018, 11:18 AM

Peter has a good point that .rsrc should come after any sections not named in the list in my first patch. Also we should probably make sure .reloc really comes last too.

Rather than just moving .rsrc to the end though, I think there is a point in trying to match link.exe's section order.

I've uploaded a new patch version. Please let me know what you think.

pcc added inline comments.Apr 4 2018, 11:23 AM
COFF/Writer.cpp
678

Not sure that I agree, it seems a little cargo cultish to me. But I'll leave it up to you folks.

685

You can probably merge this into the stable_sort like I was doing in D45273.

zturner added a subscriber: zturner.EditedApr 4 2018, 11:23 AM

Feel free to let me know if this comment is off topic here, but I was just investigating a similar issue. I have an object file with this section list:

0 .text         0000001c 0000000000000000 TEXT
1 .data         00000000 0000000000000000 DATA
2 .bss          00000000 0000000000000000 BSS
3 .xdata        00000008 0000000000000000 DATA
4 .drectve      00000030 0000000000000000
5 .debug$S      00000144 0000000000000000 DATA
6 .debug$T      00000044 0000000000000000 DATA
7 .pdata        0000000c 0000000000000000 DATA

When I link this with lld, I get this section list:

0 .pdata        0000000c 0000000140001000 DATA
1 .text         0000001c 0000000140002000 TEXT
2 .xdata        00000008 0000000140003000 DATA
3 .rdata        00000063 0000000140004000 DATA

When I link with link.exe, I get this section list:

0 .text         0000001c 0000000140001000 TEXT
1 .rdata        00000134 0000000140002000 DATA
2 .pdata        0000000c 0000000140003000 DATA

First of all, using link.exe .pdata comes at the end (or maybe more accurately, it appears in the same order as it does in the object file).

Secondly, we are outputting a .xdata section where link.exe is not.

Are these fixes also within the scope of this patch?

pcc added a comment.Apr 4 2018, 11:26 AM

Feel free to let me know if this comment is off topic here, but I was just investigating a similar issue. I have an object file with this section list:

0 .text         0000001c 0000000000000000 TEXT
1 .data         00000000 0000000000000000 DATA
2 .bss          00000000 0000000000000000 BSS
3 .xdata        00000008 0000000000000000 DATA
4 .drectve      00000030 0000000000000000
5 .debug$S      00000144 0000000000000000 DATA
6 .debug$T      00000044 0000000000000000 DATA
7 .pdata        0000000c 0000000000000000 DATA

When I link this with lld, I get this section list:

0 .pdata        0000000c 0000000140001000 DATA
1 .text         0000001c 0000000140002000 TEXT
2 .xdata        00000008 0000000140003000 DATA
3 .rdata        00000063 0000000140004000 DATA

When I link with link.exe, I get this section list:

0 .text         0000001c 0000000140001000 TEXT
1 .rdata        00000134 0000000140002000 DATA
2 .pdata        0000000c 0000000140003000 DATA

First of all, using link.exe .pdata comes at the end (or maybe more accurately, it appears in the same order as it does in the object file).

Secondly, we are outputting a .xdata section where link.exe is not.

Are these fixes also within the scope of this patch?

The second one sounds like D40197 which was reverted.

ruiu accepted this revision.Apr 4 2018, 11:26 AM

LGTM

COFF/Writer.cpp
660–662

This is a nit but I'd perhaps use 99, 100 and 50 or something like that for these numbers instead of super large numbers.

ruiu added a comment.Apr 4 2018, 11:29 AM

Oh I didn't notice that the discussion was going on while reviewing.

As to mimicking the order of MSVC linker's section output, it is indeed a curgo cult, but I'd probably do that if I start writing a Windows linker now, as the format is not entirely documented, and many tools are proprietary and can't change.

pcc added inline comments.Apr 4 2018, 11:35 AM
COFF/Writer.cpp
661

Have you confirmed that UpdateResource updates the .reloc address in the data directory if it is moved?

hans added inline comments.Apr 4 2018, 11:38 AM
COFF/Writer.cpp
660–662

Will do.

661

Let me check..

hans added inline comments.Apr 4 2018, 11:42 AM
COFF/Writer.cpp
661

Yes it looks like it does (I compared Chromium's mini_installer.exe and next_version_mini_installer.exe)

hans added a comment.Apr 4 2018, 12:17 PM

I'll go ahead and commit. Please feel free to follow up in your timezone if you like to make any changes.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Apr 4 2018, 12:25 PM

Thanks for getting this and grinding through and updating the test cases!