This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Implement constructor priorities
ClosedPublic

Authored by mstorsjo on Nov 24 2017, 12:52 AM.

Details

Summary

The priorities in the section name suffixes are zero padded, allowing the linker to just do a lexical sort.

Add zero padding for .ctors sections in ELF as well.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Nov 24 2017, 12:52 AM
mstorsjo added a reviewer: rnk.
ruiu edited edge metadata.Nov 24 2017, 1:24 AM

Could you give me a context of this patch? .ctors and .dtors are essentially an ELF stuff, so I'm wondering how you are going to use it in COFF.

mstorsjo updated this revision to Diff 124312.Nov 26 2017, 2:13 PM
mstorsjo edited the summary of this revision. (Show Details)

I noticed that the .ctors.1234 numerical suffixes are zero padded by GCC when targeting MinGW, updated this patch accordingly and expanded the test to showcase it.

It doesn't seem like any other modern platform actually defaults to using .ctors at all, so I'm not sure if other platforms should have zero padding or not. If linking an object file that contains unpadded .ctors with GNU ld, they are interpreted numerically (and end up in .init_array). If linking for MinGW with GNU ld though, the zero padding is needed for them to be ordered properly.

ruiu added inline comments.Nov 27 2017, 10:06 AM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1225–1229 ↗(On Diff #124312)

Maybe std::string Name = IsCtor ? ".ctors" : ".dtors" is slightly more readable?

test/CodeGen/X86/constructor.ll
33 ↗(On Diff #124312)

Is this correct? I thought it would be .ctors.09980.

mstorsjo added inline comments.Nov 27 2017, 2:02 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1225–1229 ↗(On Diff #124312)

Sure, I can change that.

1233 ↗(On Diff #124312)

Do you have any suggestion on a more idiomatic way of doing the equivalent of snprintf("%05u", 65535 - Priority) here? This feels quite roundabout.

test/CodeGen/X86/constructor.ll
33 ↗(On Diff #124312)

I'm actually not sure. My GCC reference doesn't output .ctors on linux (and can't easily be made to output it either, it seems). GNU ld does handle it correctly with .ctors without the zero padding on linux, but not for mingw though. But I found this GCC bug report, which discussed similar matters, and it does seem like it did do zero padding there as well: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770

So I guess it makes sense to make it consistent and add the zero padding there as well. If the linker does interpret it numerically, it will still work, and if it doesn't, zero padding is the only way it can work.

ruiu added inline comments.Nov 27 2017, 2:10 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1233 ↗(On Diff #124312)

I think snprintf isn't actually bad. Alternatively, you could include llvm/Support/Format.h and do

Number << format("%05u", 65535 - Priority);

. Or you could keep the current code. I don't have a strong preference.

mstorsjo added inline comments.Nov 27 2017, 2:29 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1233 ↗(On Diff #124312)

I did find llvm/Support/Format.h, but that doesn't work with the << operator directly to a std::string. After yet some more grepping around the source base, I found raw_string_ostream, which together with format() probably is the best match of how I want to write it.

mstorsjo updated this revision to Diff 124473.Nov 27 2017, 2:36 PM
mstorsjo edited the summary of this revision. (Show Details)

Added zero padding for ELF as well, rewrote the zero padding in a more idiomatic way, applied Rui's style suggestion.

ruiu accepted this revision.Nov 27 2017, 2:40 PM

LGTM

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
535–536 ↗(On Diff #124473)

nit: I believe you could write this as

raw_string_ostream(Name) << format(".%05u", 65535 - Priority);
1226–1227 ↗(On Diff #124473)

Ditto

This revision is now accepted and ready to land.Nov 27 2017, 2:40 PM
mstorsjo added inline comments.Nov 27 2017, 2:54 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
535–536 ↗(On Diff #124473)

Oh, yes - thanks!

This revision was automatically updated to reflect the committed changes.