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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
