Read machO sections section by section #37043
Conversation
This changes how ReflectionContext reads machO reflection sections by reading them individually instead of as one big memory block spanning from the first to the last section (and including whatever else is in between). This change will enable an optimization on LLDB's side where, if we're reading read-only data, we read from the file-cache instead of the child process, which should speed up debugging when working with remote processes.
|
@swift-ci Please test |
|
@swift-ci Please smoke test |
|
The only downside I can see in this patch is that we are making ~5x more individual calls to the MemoryReader (one per section instead of 1 per file) but that would seem to be the right trade-off for the majority of use-cases. |
|
LGTM. I think the existing "read one big chunk" scheme was meant to cut down on calls to lldb's memory reader because it was faster to do it that way at that time. If it's now faster to make separate calls, let's do it! |
| @@ -244,11 +215,13 @@ class ReflectionContext | |||
| if (strncmp(S->sectname, Name.data(), strlen(Name.data())) != 0) | |||
There was a problem hiding this comment.
Not your code, but it's inefficient to call strlen(StringRef.data()), this should be Name.size() instead.
There was a problem hiding this comment.
Perhaps you could fix that in a separate follow-up commit?
| if (!LocalSectBuf) | ||
| return {nullptr, 0}; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
There's a comment in a test that says __swift5_reflstr is optional, possibly stripped:
There was a problem hiding this comment.
That logic skips the image only if all sections are missing. If any are present, then it gets added.
There was a problem hiding this comment.
can I delete these comments so nobody sees how poor my reading comprehension is?
There was a problem hiding this comment.
Sure, but we've all been there, no worries.
There was a problem hiding this comment.
yup! I was kidding about deleting :D
This changes how ReflectionContext reads machO reflection sections by reading them individually instead of as one big memory block spanning from the first to the last section (and including whatever else is in between). This change will enable an optimization on LLDB's side where, if we're reading read-only data, we read from the file-cache instead of the child process, which should speed up debugging when working with remote processes.