[Lldb-commits] [PATCH] D64013: Correctly use GetLoadedModuleList to take advantage of libraries-svr4

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:185-207
+  if (
+      // 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.
+      (m_previous.state == eConsistent && m_current.state == eConsistent) ||
+      // 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.
----------------
aadsm wrote:
> labath wrote:
> > 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:
> >   @#%@#%!@%
> > }
> > ```
> 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?
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...


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2383
+          LLDB_LOG_ERROR(log, std::move(error),
+                         "ProcessGDBRemote::{1} failed to load modules: {0}",
+                         __FUNCTION__);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64013/new/

https://reviews.llvm.org/D64013