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

[António Afonso via Phabricator via lldb-commits]

aadsm marked an inline comment as done.
aadsm added inline comments.


================
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.
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64013