Page MenuHomePhabricator

Correctly use GetLoadedModuleList to take advantage of libraries-svr4
ClosedPublic

Authored by aadsm on Jul 1 2019, 8:05 AM.

Details

Summary

Here's a replacement for D62504. I thought I could use LoadModules to implement this but in reality I can't because there are at few issues with it:

  • The LoadModules assumes that the list returned by GetLoadedModuleList is comprehensive in the sense that reflects all the mapped segments, however, this is not true, for instance VDSO entry is not there since it's loaded manually by LoadVDSO using GetMemoryRegionInfo and it doesn't represent a specific shared object in disk. Because of this LoadModules will unload the VDSO module.
  • The loader (interpreter) module might have also been loaded using GetMemoryRegionInfo, this is true when we launch the process and the rendezvous structure is not yet available (done through LoadInterpreterModule()). The problem here is that this entry will point to the same file name as the one found in /proc/pid/maps, however, when we read the same module from the r_debug.link_map structure it might be under a different name. This is true at least on CentOS where the loader is a symlink. Because of this LoadModules will unload and load the module in a way where the rendezvous breakpoint is unresolved but not resolved again (because we add the new module first and remove the old one after).

The symlink issue might be fixable by first unloading the old and loading the news (but sounds super brittle), however, I'm not sure how to fix the VDSO issue.
Since I can't trust it I'm just going to use GetLoadedModuleList directly with the same logic that we use today for when we read the linked list in lldb. The only safe thing to do here is to only calculate differences between different snapshots of the svr4 packet itself. This will also cut the dependency this plugin has from LoadModules.

I separated the 2 logics into 2 different functions (remote and not remote) because I don't like mixing 2 different logics in the same function with if/else's. Two different functions makes it easier to reason with I believe. However, I did abstract away the logic that decides if we should take a snapshot or add/remove modules so both functions could reuse it.

The other difference between the two is that on the UpdateSOEntriesFromRemote I take the snapshot only once when state = Consistent because I didn't find a good reason to always update that, as we already got the list from state = Add | Remove. I probably should use the same logic on UpdateSOEntries though I don't see a reason not to since it's really using the same data, just read in different places. Any thoughts here?

It might also be worthwhile to add a test to make sure we don't unload modules that were not actually "unloaded" like the vdso. I haven't done this yet though.
This diff is also missing the option for svr4 like proposed in https://reviews.llvm.org/D62503#1564296, I'll start working on this but wanted to have this up first.

Diff Detail

Repository
rL LLVM

Event Timeline

aadsm created this revision.Jul 1 2019, 8:05 AM
labath added a comment.Jul 1 2019, 8:44 AM

I don't have time to do a full review of this today, but this seems pretty good at a first glance...

lldb/include/lldb/Target/Process.h
684 ↗(On Diff #207315)

Since you're already changing the signature, let's make this return an llvm::Error.

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
184–206 ↗(On Diff #207315)

I like that you've factored out the decoding of the states, but I think it would be better to make this a separate function returning an enum (GetAction ?). This will make it more obvious which combinations of flags are covered. Also, StateIsTakeSnapshot reads weirdly...

aadsm marked 2 inline comments as done.Jul 1 2019, 9:33 AM
aadsm added inline comments.
lldb/include/lldb/Target/Process.h
684 ↗(On Diff #207315)

Ah yeah ok. It still weirds me out that an Error object might not be an error. In this regard I actually much prefer the Status object as it much better conveys the meaning of the return object. Is the plan to move from Status to Error in lldb?

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
184–206 ↗(On Diff #207315)

Sounds good. I didn't want to use the word consistent, I wanted to match rest of the language used in the code. Originally I had ShouldX (e.g.: ShouldTakeSnapshot) but I didn't think it was clear when to call it (or that it was derived from the state). I like the GetAction idea, thanks.

labath added inline comments.Jul 1 2019, 11:46 AM
lldb/include/lldb/Target/Process.h
684 ↗(On Diff #207315)

Yep. lldb_private::Status used to be called lldb_private::Error, but it was renamed to avoid confusing with the llvm version.

aadsm updated this revision to Diff 207455.Jul 1 2019, 6:17 PM

Address comments, add GetAction that returns an enum with what to do.

Looks mostly good. Just a couple of style comments.

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
185–207 ↗(On Diff #207455)

This logic is pretty hard to follow. I'd suggest trying to structure as some switch statements. Something like:

switch(current_state) {
case Consistent:
  switch (previous_state) {
    case Consistent: return TakeSnapshot;
    case Add: return Add;
    case Remove: return Remove;
  }
case Add: case Remove:
  @#%@#%!@%
}
240 ↗(On Diff #207455)

Use a switch here.

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
265 ↗(On Diff #207455)

We don't usually use the enum tag here.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
2731 ↗(On Diff #207455)

You'll need to somehow handle the case when GetLoadedModuleList fails (maybe by logging the error).

aadsm marked an inline comment as done.Jul 17 2019, 9:58 AM
aadsm added inline comments.
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
185–207 ↗(On Diff #207455)

Interesting, I actually think it's easier to follow than breaking it down to nested switches. I wanted to code to reflect the mental model that decides which action to take.

That's why I structured it in the way of:

if (state A) do action 1
if (state B) do action 2
...

The switch statement basically creates a logic decision tree that I personally find harder to follow. If I want to know what states makes us take a snapshot I need to search the entire code for return eTakeSnapshot and then go up the nested switch for each one of them and collect all the cases. In the other one I can focus my attention just to the condition for "do action 1".

Here's how it looks like:

switch (m_current.state) {
    case eConsistent:
      switch (m_previous.state) {
        case eConsistent: return eTakeSnapshot;
        case eAdd: return eAddModules;
        case eDelete: return eRemoveModules;
      }
      break;
    case eAdd:
    case eDelete: 
      switch (m_previous.state) {
        case eConsistent: return eTakeSnapshot;
        case eAdd: if (m_current.state == eDelete)
          return eTakeSnapshot;
      }
  }
  
  return eNoAction;

I also wanted to make the hacks a bit more obvious so I came up with this which I think strikes a good balance in making the decision clear and isolates the hack we have for android loaders:

if (m_current.state == eConsistent) {
    switch (m_previous.state) {
      // When the previous and current states are consistent this is the first
      // time we have been asked to update.  Just take a snapshot of the
      // currently loaded modules.
      case eConsistent: return eTakeSnapshot;
      // If we are about to add or remove a shared object clear out the current
      // state and take a snapshot of the currently loaded images.
      case eAdd: return eAddModules;
      case eDelete: return eRemoveModules;
    }
    return eNoAction;
  }
  
  if (m_current.state == eAdd || m_current.state == eDelete) {
    // Some versions of the android dynamic linker might send two
    // notifications with state == eAdd back to back. Ignore them until we
    // get an eConsistent notification.
    if (!(m_previous.state == eConsistent ||
          (m_previous.state == eAdd && m_current.state == eDelete)))
      return eNoAction;

    return eTakeSnapshot;
  }
  
  return eNoAction;

Thoughts?

aadsm updated this revision to Diff 210413.Jul 17 2019, 2:15 PM

Address comments.
In the end I used nested switches for the GetAction function. I thought about it and I guess it boils down to personal preference. It's still harder for me to read than the if version so I put some empty lines to improve that.

aadsm updated this revision to Diff 210422.Jul 17 2019, 2:30 PM

Missed one function

labath accepted this revision.Jul 18 2019, 2:06 AM

lgtm

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
185–207 ↗(On Diff #207455)

This would be fine too. I was mainly concerned about the huge if condition with embedded comments. I was staring at it for 10 minutes, and I still wasn't sure I understood the conditions it would fire in...

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
2383 ↗(On Diff #210422)

btw, LLDB_LOG macros are smart enough to embed file and function information into the log message themselves (if you use "log enable -F"), so you don't need to repeat it here.

This revision is now accepted and ready to land.Jul 18 2019, 2:06 AM
aadsm updated this revision to Diff 211141.Jul 22 2019, 10:26 AM

Address comments, also checks the LoadModules return on DynamicLoaderWindowsDYLD

aadsm updated this revision to Diff 211241.Jul 22 2019, 7:55 PM

Handle error returned by GetLoadedModuleList

labath accepted this revision.Jul 23 2019, 1:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jul 25, 7:27 AM