cas.o: Improve Block sorting for ELF object ingestion stability

[cachemeifyoucan]

For ELF object files, it can create blocks in the same section that are
almost identical except relocations inside the block (e.g. .init_array
section). This can cause indeterministic output when converting ELF
object files. Fix the problem by recursively looking into relocation
targets if needed when sorting blocks/edges.

[cachemeifyoucan]

Now I think about this... If you carefully craft the object file, you can get sorting algorithm to infinite loop. I need to add a maximum depth probably.

[cachemeifyoucan]

Alternative is we can create ELF blocks using its section offset in the file and assign Address 0 to those sections with NOBITS attributes.

[dexonsmith]

Please push a standalone commit that moves these to ObjectFormatHelpers that has NFC (don't think it needs review, just push it) and then rebase so it's easy to see what's actually changing here.

I'm nervous about making the comparison recursive.

What about @lhames's idea to use Section::getOrdinal()? I.e., change "compare-by-address" to actually compare:

• Section::getOrdinal() then
• Block::getAddress()

[cachemeifyoucan]

Please push a standalone commit that moves these to ObjectFormatHelpers that has NFC (don't think it needs review, just push it) and then rebase so it's easy to see what's actually changing here.

I'm nervous about making the comparison recursive.

What about @lhames's idea to use Section::getOrdinal()? I.e., change "compare-by-address" to actually compare:

• Section::getOrdinal() then
• Block::getAddress()

I want to avoid that as well but there is no other option other than make blocks to have deterministic ordering in Section (using a vector, instead of set).

The indeterminism I try to fix here are the blocks for the same section, with no address assigned.

[lhames]

The indeterminism I try to fix here are the blocks for the same section, with no address assigned.

What about assigning an initial layout in the LinkGraphBuilder?

As long as no ELF semantics rely on symbols initially having a null address this seems like it should be safe.

[cachemeifyoucan]

The indeterminism I try to fix here are the blocks for the same section, with no address assigned.

What about assigning an initial layout in the LinkGraphBuilder?

As long as no ELF semantics rely on symbols initially having a null address this seems like it should be safe.

So I tried an easy patch by using the offset in object file as the address of the block (except those zerofill block gets address 0 since they don't occupy any space in object file) but it turns out it requires more work. I forgot the exact reason but I can easily recreate that and report back. I remember I ditched the idea because the work for building an initial layout in the object file is unnecessary for the use of ELF object in JIT context so I am not sure if we want to pay that cost.

[cachemeifyoucan]

diff --git a/llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h b/llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h
index f8339be67ea0..c8a6d31073b3 100644
--- a/llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h
+++ b/llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h
@@ -328,7 +328,7 @@ template <typename ELFT> Error ELFLinkGraphBuilder<ELFT>::graphifySections() {
       if (!Data)
         return Data.takeError();

-      B = &G->createContentBlock(*GraphSec, *Data, Sec.sh_addr,
+      B = &G->createContentBlock(*GraphSec, *Data, Sec.sh_offset,
                                  Sec.sh_addralign, 0);
     } else
       B = &G->createZeroFillBlock(*GraphSec, Sec.sh_size, Sec.sh_addr,
diff --git a/llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp b/llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
index 7c7508642ec5..04bf1184fb67 100644
--- a/llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
+++ b/llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
@@ -246,7 +246,10 @@ private:
     }
     }

-    JITTargetAddress FixupAddress = FixupSection.sh_addr + Rel.r_offset;
+    JITTargetAddress FixupBase = FixupSection.sh_type == ELF::SHT_NOBITS
+                                     ? FixupSection.sh_addr
+                                     : FixupSection.sh_offset;
+    JITTargetAddress FixupAddress = FixupBase + Rel.r_offset;
     Edge::OffsetT Offset = FixupAddress - BlockToFix.getAddress();
     Edge GE(Kind, Offset, *GraphSymbol, Addend);
     LLVM_DEBUG({

@lhames This is the cheapest way to give a layout for most of the content in the object file. Does this look ok to you?

[lhames]

I think that should work, but it might be better to assign new addresses to the blocks as a post-processing step on the graph rather than modifying the parser -- that approach should be less sensitive to future changes to the parser.

[cachemeifyoucan]

I think that should work, but it might be better to assign new addresses to the blocks as a post-processing step on the graph rather than modifying the parser -- that approach should be less sensitive to future changes to the parser.

It is too late when it turns into the LinkGraph since the offset information is lost and there won't be deterministic ordering for visiting blocks (the problem we try to solve here) so the address assigned will not be deterministic. Let me know if I missed anything.

[shahmishal]

@swift-ci test

[lhames]

It is too late when it turns into the LinkGraph since the offset information is lost and there won't be deterministic ordering for visiting blocks (the problem we try to solve here) so the address assigned will not be deterministic. Let me know if I missed anything.

No, you're right -- doing this in the LinkGraph will be too late for your purposes. I'm back to thinking that the parser is the right place to implement this.

So I tried an easy patch by using the offset in object file as the address of the block (except those zerofill block gets address 0 since they don't occupy any space in object file) but it turns out it requires more work. I forgot the exact reason but I can easily recreate that and report back. I remember I ditched the idea because the work for building an initial layout in the object file is unnecessary for the use of ELF object in JIT context so I am not sure if we want to pay that cost.

Ok. We should have a talk about determinism for JITLink plugins -- we haven't promised that to clients yet, but maybe we'll want it in the future, in which case this cost will have to be paid anyway. It's also likely to be pretty cheap relative to everything else that JITLink and ORC are doing.

[dexonsmith]

Please push a standalone commit that moves these to ObjectFormatHelpers that has NFC (don't think it needs review, just push it) and then rebase so it's easy to see what's actually changing here.

(BTW, if you've pushed something, maybe you need to rebase, since I still can't easily tell what's changed... but maybe you're not ready for a review yet anyway...)

I'm nervous about making the comparison recursive.
I want to avoid that as well but there is no other option other than make blocks to have deterministic ordering in Section (using a vector, instead of set).

Or a SetVector.

Or, add an BlockID field to Block that is deterministically assigned by LinkGraph in createBlock() in order of creation (i.e., LinkGraph::NextBlockID starts at 0). Two options for how to use it:

1. Keep the field private, but expose ajitlink::Block::operator<() that compares based on the BlockID.
2. Add a public accessor.