This is an archive of the discontinued LLVM Phabricator instance.

ELF: Implement the correct semantics of .[cd]tors.
ClosedPublic

Authored by ruiu on Feb 10 2016, 6:59 PM.

Details

Summary

As I noted in the comment, the sorting order of .[cd]tors are
different from .{init,fini}_array's.

Diff Detail

Event Timeline

ruiu updated this revision to Diff 47577.Feb 10 2016, 6:59 PM
ruiu retitled this revision from to ELF: Implement the correct semantics of .[cd]tors..
ruiu updated this object.
ruiu added a reviewer: silvas.
ruiu added a subscriber: llvm-commits.
davide added a subscriber: davide.Feb 10 2016, 7:07 PM

Some comments.

ELF/OutputSections.cpp
802

Can you name '3' instead of using the magic number?

835

Drive by comment: not a huge fan of X and Y as variable names. Sure, their scope is narrow but if you can come up with something more self-explicative, it would be better.

842

It is not obvious to me why you need stable sorting. Do you mind adding a comment?

silvas edited edge metadata.Feb 10 2016, 7:11 PM

Can you post your test scripts that you used to verify correct behavior?

Bigcheese requested changes to this revision.Feb 10 2016, 7:23 PM
Bigcheese added a reviewer: Bigcheese.
Bigcheese added a subscriber: Bigcheese.
Bigcheese added inline comments.
ELF/OutputSections.cpp
797

You also need to check for crtend. Its .ctors sections need to go after other .ctors.N sections.

test/ELF/ctors_dtors_priority.s
35

This should be:

1, 4, 5, 3, 2

.ctors is evaluated from the end to the start.

This revision now requires changes to proceed.Feb 10 2016, 7:23 PM
ruiu updated this revision to Diff 47580.Feb 10 2016, 7:23 PM
ruiu edited edge metadata.
  • Added a test for the crtbegin.o rule.
ELF/OutputSections.cpp
802

We can name it SuffixLen or something like that, but it is probably too narrow. Added a comment instead.

835

I rather prefer this way. I actually tried to name NameA and NameB, but X and Y grows on me as I modified this code many times before sending for the code review.

842

Added a comment to the function for init/fini.

Bigcheese added inline comments.Feb 10 2016, 7:25 PM
ELF/OutputSections.cpp
802

3 is pretty obvious here, I can't think of a name that would provide any clarification. The function has a comment stating what the regex is.

ruiu updated this revision to Diff 47584.Feb 10 2016, 7:48 PM
ruiu edited edge metadata.
  • Address Michael's comments.
  • Added a test for crtend.

GNU ld 2.22 has the following default linker script for .ctors/.dtors sections. I am sure that other versions have the same declarations.

.ctors          :
{
  KEEP (*crtbegin.o(.ctors))
  KEEP (*crtbegin?.o(.ctors))
  /* We don't want to include the .ctor section from
     the crtend.o file until after the sorted ctors.
     The .ctor section from the crtend file contains the
     end of ctors marker and it must be last */
  KEEP (*(EXCLUDE_FILE (*crtend.o *crtend?.o ) .ctors))
  KEEP (*(SORT(.ctors.*)))
  KEEP (*(.ctors))
}
.dtors          :
{
  KEEP (*crtbegin.o(.dtors))
  KEEP (*crtbegin?.o(.dtors))
  KEEP (*(EXCLUDE_FILE (*crtend.o *crtend?.o ) .dtors))
  KEEP (*(SORT(.dtors.*)))
  KEEP (*(.dtors))
}

So the order should be follow:

  1. .ctors section from *crtbegin.o
  2. .ctors section from *crtbegin?.o
  3. .ctors sections from other object files except *crtend.o and *crtend?.o
  4. .ctors.* from any files sorted by priority
  5. .ctors sections from *crtend.o and *crtend?.o

As far as I can see now .ctors/.dtors sections from the crtend.o go first.

ruiu updated this revision to Diff 47727.Feb 11 2016, 2:00 PM
  • Address Simon's comment.
ruiu added a subscriber: ruiu.Feb 11 2016, 2:01 PM

Tried to produce the same result as the linker script. Can you take a look?

atanasyan accepted this revision.Feb 11 2016, 2:05 PM
atanasyan added a reviewer: atanasyan.

LGTM Thanks.

Bigcheese accepted this revision.Feb 11 2016, 3:22 PM
Bigcheese edited edge metadata.

Fixes the original testcase and lgtm.

This revision is now accepted and ready to land.Feb 11 2016, 3:22 PM
ruiu closed this revision.Feb 11 2016, 3:46 PM

Committed as r260620.