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