[Reflection] Add support for reading ELF. #17771

dcci · 2018-07-05T21:13:53Z

This ports Mike's work for the new reflection API to ELF.
This was discussed in abstract several times and I decided to prototype it to see how it looked like.
It needs some cleanups and I need to make sure all the tests pass, but I got to a checkpoint where the basic structure is all there, so I decided it could be a good idea to have another pair of eyes on it.

mikeash · 2018-07-05T21:23:48Z

include/swift/Reflection/ReflectionContext.h

+      return false;
+    auto ReflStrMdSec = findELFSectionByName("swift5_reflstr");
+    if (ReflStrMdSec.first == nullptr || ReflStrMdSec.second == nullptr)
+      return false;


For Mach-O, we succeed if any of the sections are found, as at least some of them are optional. I think ELF should probably do the same. That would also allow you to have this code read swift5_builtin and swift5_capture, and then it will start reading those whenever the compiler starts emitting them.

I think the semantics that Mach-O gives are fine (for now), but we might want to revisit them in the future.
If, e.g. for some reason the compiler doesn't emit the typeref section or the reflstr section then we're in big trouble because the debugger kind-of relies on them to be present. I imagine in that case you might at least emit a warning and degrade gracefully in lldb, but I also understand this is a discussion for another day.

[... or similarly, somebody runs the executable through strip or objcopy which drops these sections on the floor]

I guess it depends on whether you want best effort or all-or-nothing. Might depend on the API’s client. I think some of these sections really are optional, at least I’ve seen freshly built binaries that don’t have them all.

If the compiler has nothing to emit for any of the sections, it probably ends up being omitted entirely from the binary. For that reason, I think that it's best to consider them all as optional, or treat the associated section as effectively empty if it doesn't exist in the binary in question. We should treat the lack of a section when the debugger expects it as a format error further down at the point we find out we need it, IMO.

mikeash · 2018-07-05T21:24:21Z

Hooray, ELF support!

dcci · 2018-07-06T18:59:09Z

@mikeash This passes all the tests, so I guess it's in a mergeable state. Can you please take another look while I run the tests?

dcci · 2018-07-06T18:59:15Z

@swift-ci please test

gottesmm · 2018-07-06T18:59:45Z

@compnerd do you have any thoughts here?

swift-ci · 2018-07-06T19:01:24Z

Build failed
Swift Test Linux Platform
Git Sha - 4468bc7409fbe7de027338b3a01c4c1e19dbd3dd

swift-ci · 2018-07-06T20:28:14Z

Build failed
Swift Test OS X Platform
Git Sha - db180400267c8ce9526026cc2395279d4abf47d1

dcci · 2018-07-06T20:36:05Z

@mikeash this seems to be a bug in the existing support, unfortunately :|

Test Result (6 failures / +5)
Swift(iphonesimulator-i386).Reflection.typeref_lowering.swift
Swift(iphonesimulator-i386).Reflection.typeref_decoding_objc.swift
Swift(iphonesimulator-i386).Reflection.typeref_decoding_imported.swift
Swift(iphonesimulator-i386).Reflection.typeref_decoding.swift
Swift(iphonesimulator-i386).Reflection.capture_descriptors.sil
Swift(iphonesimulator-i386).Reflection.box_descriptors.sil

dcci · 2018-07-13T17:11:39Z

@swift-ci please test osx

dcci · 2018-07-13T17:12:06Z

@swift-ci please test macos

swift-ci · 2018-07-13T18:42:24Z

Build failed
Swift Test OS X Platform
Git Sha - db180400267c8ce9526026cc2395279d4abf47d1

compnerd · 2018-07-17T19:48:27Z

include/swift/Reflection/ReflectionContext.h

-
+#else // ELF platforms.
+  bool addImage(RemoteAddress ImageStart) {
+    auto Buf = this->getReader().readBytes(ImageStart, sizeof(Elf64_Ehdr));


I think that we shouldn't assume 64-bitness. 32-bit ARM is pretty common still with Linux. Byte 5 should identify the correct bitsex: 1 -> 32-bit, 2 -> 64-bit. This is technically named EI_CLASS.

My understanding is that there's no 32-bit support for swift (Linux).
As such, I think we can generalize this as a follow up. Probably we should have an assertion "unimplemented" for anything that's 32-bits?

Um, pretty sure that the x86 and ARM support in 32-bit is reasonably there :)

compnerd · 2018-07-17T19:49:24Z

include/swift/Reflection/ReflectionContext.h

+      return false;
+
+    // From the header, grab informations about the section header table.
+    auto SectionHdrAddress = ImageStart.getAddressData() + Hdr->e_shoff;


This is definitely broken with 32-bit images. offsetof(Elf_Ehdr, e_shoff) is going to be 0x20 vs 0x28. It also is a 4-byte vs 8-byte value.

I understand this all is 64-bit specific. We can generalize to 32-bits later.

compnerd · 2018-07-17T19:49:46Z

include/swift/Reflection/ReflectionContext.h

+
+    // From the header, grab informations about the section header table.
+    auto SectionHdrAddress = ImageStart.getAddressData() + Hdr->e_shoff;
+    auto SectionHdrNumEntries = Hdr->e_shnum;


Again broken: offsetof(Elf_EHdr, e_shnum) is 0x30 vs 0x3c.

compnerd · 2018-07-17T19:50:33Z

include/swift/Reflection/ReflectionContext.h

+    // From the header, grab informations about the section header table.
+    auto SectionHdrAddress = ImageStart.getAddressData() + Hdr->e_shoff;
+    auto SectionHdrNumEntries = Hdr->e_shnum;
+    auto SectionEntrySize = Hdr->e_shentsize;


Again broken: offsetof(Elf_EHdr, e_shentsize) is 0x2e vs 0x3a

compnerd · 2018-07-17T19:52:55Z

include/swift/Reflection/ReflectionContext.h

+    // This provides quick access to the section header string table index.
+    // We also here handle the unlikely even where the section index overflows
+    // and it's just a pointer to secondary storage (SHN_XINDEX).
+    uint32_t SecIdx = Hdr->e_shstrndx;


Again broken: offsetof(Elf_EHdr, e_shstrndx) is 0x32 vs 0x3e

compnerd · 2018-07-17T19:55:42Z

include/swift/Reflection/ReflectionContext.h

+
+    const Elf64_Shdr *SecHdrStrTab = SecHdrVec[SecIdx];
+    Elf64_Off StrTabOffset = SecHdrStrTab->sh_offset;
+    Elf64_Xword StrTabSize = SecHdrStrTab->sh_size;


Both of these are wrong. offsetof(Elf_SHdr, sh_offset) is 0x10 vs 0x18. offsetof(Elf_SHdr, sh_size) is 0x14 vs 0x20. They are 3 vs 8 bytes as well.

compnerd · 2018-07-17T19:56:13Z

include/swift/Reflection/ReflectionContext.h

+      return {{nullptr, nullptr}, 0};
+    };
+
+    // FIXME: there's no swift5_builtin and swift5_capture section on Linux.


This is a stale comment. I'll remove it, thanks.

compnerd · 2018-07-17T19:58:33Z

tools/swift-reflection-dump/swift-reflection-dump.cpp

@@ -263,7 +263,8 @@ static int doDumpReflectionSections(ArrayRef<std::string> binaryFilenames,
    objectOwners.push_back(std::move(objectOwner));
    objectFiles.push_back(objectFile);

-    context.addReflectionInfo(findReflectionInfo(objectFile));
+    auto startAddress = (uintptr_t)objectFile->getData().begin();


C++ style casts please.

compnerd · 2018-07-17T22:50:37Z

It really is unfortunate, but, yes, this is the right approach. For remote inspection of binaries, we can't just access the internal special variables to find the section bounds and have to look up the sections manually :-(. This is a super expensive solution, but I think that the best way to handle it for ELF.

dcci · 2018-07-18T22:27:22Z

@swift-ci please test

I and Saleem agreed 32-bits can be done as followup offline.

swift-ci · 2018-07-18T23:36:36Z

Build failed
Swift Test OS X Platform
Git Sha - db180400267c8ce9526026cc2395279d4abf47d1

swift-ci · 2018-07-18T23:36:37Z

Build failed
Swift Test Linux Platform
Git Sha - db180400267c8ce9526026cc2395279d4abf47d1

<rdar://problem/41901725>

dcci · 2018-07-19T22:22:46Z

Finally ready.
@swift-ci test and merge

<rdar://problem/41901725>

dcci · 2018-07-19T22:25:07Z

@swift-ci test and merge

clackary · 2018-07-19T22:27:58Z

@swift-ci test and merge

dcci requested review from slavapestov, jckarter, mikeash, fredriss and bob-wilson July 5, 2018 21:13

mikeash reviewed Jul 5, 2018

View reviewed changes

dcci force-pushed the remotemirrorself branch from d5190c1 to db18040 Compare July 6, 2018 18:58

dcci changed the title ~~[WIP/Reflection] Add support for reading ELF.~~ [Reflection] Add support for reading ELF. Jul 6, 2018

dcci requested a review from compnerd July 6, 2018 20:23

compnerd previously requested changes Jul 17, 2018

View reviewed changes

[RemoteMirrors] Add support for reading ELF.

a907398

<rdar://problem/41901725>

dcci force-pushed the remotemirrorself branch from db18040 to a907398 Compare July 19, 2018 22:22

[ELF] Remove a wrong comment and clarify another one.

e9d8e0a

<rdar://problem/41901725>

swift-ci merged commit 4158517 into swiftlang:master Jul 20, 2018

[Reflection] Add support for reading ELF. #17771

[Reflection] Add support for reading ELF. #17771

Uh oh!

Conversation

dcci commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeash commented Jul 5, 2018

Uh oh!

dcci commented Jul 6, 2018

Uh oh!

dcci commented Jul 6, 2018

Uh oh!

gottesmm commented Jul 6, 2018

Uh oh!

swift-ci commented Jul 6, 2018

Uh oh!

swift-ci commented Jul 6, 2018

Uh oh!

dcci commented Jul 6, 2018

Uh oh!

dcci commented Jul 13, 2018

Uh oh!

dcci commented Jul 13, 2018

Uh oh!

swift-ci commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Jul 17, 2018

Uh oh!

dcci commented Jul 18, 2018

Uh oh!

swift-ci commented Jul 18, 2018

Uh oh!

swift-ci commented Jul 18, 2018

Uh oh!

dcci commented Jul 19, 2018

Uh oh!

dcci commented Jul 19, 2018

Uh oh!

clackary commented Jul 19, 2018

Uh oh!

Uh oh!