[ELF] - Support SORT_BY_INIT_PRIORITY for .ctors.*/.dtors sections in linkerscript.

[grimar]

Summary

We have a difference in behavior with bfd linkers.

bfd default script looks like:

  .init_array     :
  {
...
    KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*) SORT_BY_INIT_PRIORITY(.ctors.*)))
...
  }

SORT_BY_INIT_PRIORITY is a feature that allows sorting by priority and used for
placing to .init_array and .fini_array sections.

Currently LLD implements SORT_BY_INIT_PRIORITY behavior that is similar for both types of sections,
that is looks wrong. And it is inconsistent with how gnu linkers work here.

That change is mostly for overall correctness,
because .ctors/.dtors is very outdated feature. Though somebody may still use such kind of bfd
script for example and will have a bug.

[davide]

Pretty much every version of gas released in the last 10 years emits exclusively .init_array.
I see what you're fixing here, but is this something that can happen in practice?

[davide]

Please also note that lld doesn't really handle the case when we have mixed .ctors and .init_array from multiple object files in the non-linkerscript path.

[grimar]

In D34326#783410, @davide wrote:

Pretty much every version of gas released in the last 10 years emits exclusively .init_array.
I see what you're fixing here, but is this something that can happen in practice?

I did not find users, but may be somebody can face this and that probably not too obvious.
May be can just error out instead on such scripts. Just leaving possible issue probably not good.

[grimar]

• Fixed mistype.

[grimar]

In D34326#783412, @davide wrote:

Please also note that lld doesn't really handle the case when we have mixed .ctors and .init_array from multiple object files in the non-linkerscript path.

Sure, non-linkerscipt patch is fine and do not need anything, we generate different output sections and the problem does not exist there.
Non-script case just has different implementations for sorting .ctors/.dtors and .init_array/.fini_array

SORT_BY_INIT_PRIORITY is a feature used for placing to .init_array/.fini_array for script case.
Probably I should not have mention "mixing" here, sorry for possible confusion, it is not really important, it is just how bfd script looks like.

When we use SORT_BY_INIT_PRIORITY(.init_array.*) calculation should be reversed in compare with SORT_BY_INIT_PRIORITY(.ctors.*).
We can have script like

.init_array : {  SORT_BY_INIT_PRIORITY(.ctors.*)) }

(simplified from bfd. So no mixing happens, .init_array may have only .ctors.*.)
And here is important to have different calculation for priority basing on section name here.

That is what this patch does.

[grimar]

• Changed implementation, simplified testcase.

Looks I found finally sorting rule used by gnu linkers.
They do not sort .[cd]tors simply here like stings or like values,
inside SORT_BY_INIT_PRIORITY.

With this change bfd and LLD produces similar result for testcase.

[ruiu]

I don't know if we need this, as it implements a feature that is different from what the manual says, and no one is complaining about this.

ELF/LinkerScript.cpp

240

trim() trims the character both from both ends. That's probably not what you want.

[grimar]

In D34326#784305, @ruiu wrote:

I don't know if we need this, as it implements a feature that is different from what the manual says, and no one is complaining about this.

I think I got it. Manual says order is ascending, and that is true. Feature should implement what manual says,
And gnu linkers do that, but we don't, because priorities for sections are encoded differently:
(lib\CodeGen\TargetLoweringObjectFileImpl.cpp)

static MCSectionELF *getStaticStructorSection(MCContext &Ctx, bool UseInitArray,
                                              bool IsCtor, unsigned Priority,
                                              const MCSymbol *KeySym) {
...
  if (UseInitArray) {
    if (IsCtor) {
      Type = ELF::SHT_INIT_ARRAY;
      Name = ".init_array";
    } else {
      Type = ELF::SHT_FINI_ARRAY;
      Name = ".fini_array";
    }
    if (Priority != 65535) {
      Name += '.';
      Name += utostr(Priority);
    }
  } else {
    // The default scheme is .ctor / .dtor, so we have to invert the priority
    // numbering.
    if (IsCtor)
      Name = ".ctors";
    else
      Name = ".dtors";
    if (Priority != 65535) {
      Name += '.';
      Name += utostr(65535 - Priority);
    }
    Type = ELF::SHT_PROGBITS;
  }

So according to above I think we should:

1. Sort priorities in accending order, we already do that, but:

a) We do that correctly for .init/fini_array's
b) Do not do that correctly for .[cd]tors because before sorting should find Priority which is 65535 minus value we use as priority now.

ELF/LinkerScript.cpp

240

Oh, right. I want to use ltrim here. I was mithinking about what trim do, thanks.

[grimar]

• Updated according to my last comment.

[ruiu]

The rule this patch implements seems too odd that I don't think we should handle this. If you want to sort .ctors or .dtors, I think you can use SORT_BY_NAME. If you use SORT_BY_INIT_PRIORITY on .ctors or .dtors, that is a misuse of the feature.

[davide]

In D34326#786241, @ruiu wrote:

The rule this patch implements seems too odd that I don't think we should handle this. If you want to sort .ctors or .dtors, I think you can use SORT_BY_NAME. If you use SORT_BY_INIT_PRIORITY on .ctors or .dtors, that is a misuse of the feature.

Agree, what was trying to point out in my first post :)
My guess about the real reason why bfd has this in their default linker script is that they're trying to support ordering of mixed .ctors and .init_array and given they're always driven by their default linkerscript, unless overridden, they can't support this in a different way.

[grimar]

Ok :)

In D34326#786241, @ruiu wrote:

If you use SORT_BY_INIT_PRIORITY on .ctors or .dtors, that is a misuse of the feature.

Does it make sence to error out this case ?

[ruiu]

I wouldn't do anything for it unless an evidence that that is actually useful is shown.

[grimar]

Abandoning then.

ELF/LinkerScript.cpp

if (ID->FilePat.match(basename(S)))
           return true;
   return false;
 }
 
 // A helper function for the SORT() command.
 static std::function<bool(InputSectionBase *, InputSectionBase *)>
 getComparator(SortSectionPolicy K) {
   switch (K) {
   case SortSectionPolicy::Alignment:
     return [](InputSectionBase *A, InputSectionBase *B) {
       // ">" is not a mistake. Sections with larger alignments are placed
       // before sections with smaller alignments in order to reduce the
       // amount of padding necessary. This is compatible with GNU.
       return A->Alignment > B->Alignment;
     };
   case SortSectionPolicy::Name:
     return [](InputSectionBase *A, InputSectionBase *B) {
       return A->Name < B->Name;
     };
   case SortSectionPolicy::Priority:
     return [](InputSectionBase *A, InputSectionBase *B) {
+      // This is used for SORT_BY_INIT_PRIORITY.
+      // Which sorts sections by GCC priority attribute in ascending order.
+      // Compiler encodes priority value in section name. For .[cd]tors
+      // it is encoded as (65535 - Priority), and for .{fini,init}array it is
+      // Priority itself. So comparsion predicate is different.
+      if ((A->Name.startswith(".ctors.") || A->Name.startswith(".dtors.")) &&
+          (B->Name.startswith(".ctors.") || B->Name.startswith(".dtors.")))
+        return getPriority(A->Name) > getPriority(B->Name);
       return getPriority(A->Name) < getPriority(B->Name);
     };
   default:
     llvm_unreachable("unknown sort policy");
   }
 }
 
 // A helper function for the SORT() command.

test/ELF/linkerscript/sort-init2.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
+# RUN: echo "SECTIONS { \
+# RUN:       .init_array : { *(SORT_BY_INIT_PRIORITY(.ctors.*)) } \
+# RUN:       .fini_array : { *(SORT_BY_INIT_PRIORITY(.dtors.*)) } }" > %t1.script
+# RUN: ld.lld --script %t1.script %t1.o -o %t2
+# RUN: llvm-objdump -s %t2 | FileCheck %s
+
+# CHECK:      Contents of section .init_array:
+# CHECK-NEXT:  0301
+# CHECK:      Contents of section .fini_array:
+# CHECK-NEXT:  1311
+
+.section .ctors.5, "aw", @progbits
+  .byte 0x1
+.section .ctors.100, "aw", @progbits
+  .byte 0x3
+
+.section .dtors.5, "aw", @progbits
+  .byte 0x11
+.section .dtors.100, "aw", @progbits
+  .byte 0x13