Skip to content

Read machO sections section by section #37043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Read machO sections section by section #37043

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 7 additions & 35 deletions include/swift/Reflection/ReflectionContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,36 +205,7 @@ class ReflectionContext
return false;

auto Slide = ImageStart.getAddressData() - Command->vmaddr;
std::string Prefix = "__swift5";
uint64_t RangeStart = UINT64_MAX;
uint64_t RangeEnd = UINT64_MAX;
auto SectionsBuf = reinterpret_cast<const char *>(Sections.get());
for (unsigned I = 0; I < NumSect; ++I) {
auto S = reinterpret_cast<typename T::Section *>(
SectionsBuf + (I * sizeof(typename T::Section)));
if (strncmp(S->sectname, Prefix.c_str(), strlen(Prefix.c_str())) != 0)
continue;
if (RangeStart == UINT64_MAX && RangeEnd == UINT64_MAX) {
RangeStart = S->addr + Slide;
RangeEnd = S->addr + S->size + Slide;
continue;
}
RangeStart = std::min(RangeStart, (uint64_t)S->addr + Slide);
RangeEnd = std::max(RangeEnd, (uint64_t)(S->addr + S->size + Slide));
// Keep the range rounded to 8 byte alignment on both ends so we don't
// introduce misaligned pointers mapping between local and remote
// address space.
RangeStart = RangeStart & ~7;
RangeEnd = RangeEnd + 7 & ~7;
}

if (RangeStart == UINT64_MAX && RangeEnd == UINT64_MAX)
return false;

auto SectBuf = this->getReader().readBytes(RemoteAddress(RangeStart),
RangeEnd - RangeStart);
if (!SectBuf)
return false;

auto findMachOSectionByName = [&](llvm::StringRef Name)
-> std::pair<RemoteRef<void>, uint64_t> {
Expand All @@ -244,11 +215,13 @@ class ReflectionContext
if (strncmp(S->sectname, Name.data(), strlen(Name.data())) != 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your code, but it's inefficient to call strlen(StringRef.data()), this should be Name.size() instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could fix that in a separate follow-up commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

continue;
auto RemoteSecStart = S->addr + Slide;
auto SectBufData = reinterpret_cast<const char *>(SectBuf.get());
auto LocalSectStart =
reinterpret_cast<const char *>(SectBufData + RemoteSecStart - RangeStart);

auto StartRef = RemoteRef<void>(RemoteSecStart, LocalSectStart);
auto LocalSectBuf =
this->getReader().readBytes(RemoteAddress(RemoteSecStart), S->size);
if (!LocalSectBuf)
return {nullptr, 0};
Comment on lines +220 to +221
Copy link
Contributor

@kastiglione kastiglione Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Don't read, it's all wrong.

I was curious about this, whether a problem with one section could cause issues that previous didn't exist. It doesn't look like it, but I do see something that looks like it could be a problem in the surrounding code.

This lambda is used to read the following sections:

__swift5_fieldmd
__swift5_assocty
__swift5_builtin
__swift5_capture
__swift5_typeref
__swift5_reflstr

If any of these sections are missing or can't be read, then readMachOSections returns early:

if (FieldMdSec.first == nullptr &&
    AssocTySec.first == nullptr &&
    BuiltinTySec.first == nullptr &&
    CaptureSec.first == nullptr &&
    TypeRefMdSec.first == nullptr &&
    ReflStrMdSec.first == nullptr)
  return false;

ReflectionInfo info = {
    {FieldMdSec.first, FieldMdSec.second},
    {AssocTySec.first, AssocTySec.second},
    {BuiltinTySec.first, BuiltinTySec.second},
    {CaptureSec.first, CaptureSec.second},
    {TypeRefMdSec.first, TypeRefMdSec.second},
    {ReflStrMdSec.first, ReflStrMdSec.second}};

this->addReflectionInfo(info);

Question

I looked at my small swift program, and it has only 3 of these sections, it does not have:

  • __swift5_assocty
  • __swift5_builtin
  • __swift5_reflstr

I am not especially familiar with these sections, but I can imagine there being some images that don't use associated types.

In practice, can these sections truly be absent, and, what does this break when one of these sections is missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment in a test that says __swift5_reflstr is optional, possibly stripped:

https://github.com/apple/swift/blob/2b435f9839e10ea5901b1fa560f7bb87650bfbaa/stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift#L96

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logic skips the image only if all sections are missing. If any are present, then it gets added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh facepalm haha

Copy link
Contributor

@kastiglione kastiglione Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I delete these comments so nobody sees how poor my reading comprehension is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but we've all been there, no worries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! I was kidding about deleting :D


auto StartRef = RemoteRef<void>(RemoteSecStart, LocalSectBuf.get());
savedBuffers.push_back(std::move(LocalSectBuf));
return {StartRef, S->size};
}
return {nullptr, 0};
Expand Down Expand Up @@ -307,7 +280,6 @@ class ReflectionContext
}

savedBuffers.push_back(std::move(Buf));
savedBuffers.push_back(std::move(SectBuf));
savedBuffers.push_back(std::move(Sections));
return true;
}
Expand Down