Disabling the thread list while the target is running sounds like a pretty complex change. For example, what should happen if a Python script calls lldb.process.GetThreadAtIndex(n) while the target is running, which currently works?

The sequence I found is:

• WillResume
• DoResume sends eBroadcastBitAsyncContinue to ProcessGDBRemote::AsyncThread
• AsyncThread calls process->SetPrivateState(eStateRunning);
• …which sends eBroadcastBitStateChanged back to the main thread, handled by Process::HandlePrivateEvent
• …which ends up in this stack, when it tries to figure out whether to report the state change to the user:
  • Process::HandlePrivateEvent ->
  • Process::ShouldBroadcastEvent ->
  • ThreadList::ShouldReportRun ->
  • Thread::ShouldReportRun ->
  • ThreadPlan::ShouldReportRun ->
  • ThreadPlan::GetPreviousPlan ->
  • ThreadPlan::GetThread

In D86388#2234418, @jingham wrote:

I'm confused as to how this patch actually fixes the problem. When the thread gets removed from the thread list, it should get Destroy called on it - which should set m_destroy_called, causing IsValid to return false.. So I am not clear under what circumstances FindThreadByID will fail, but the cached thread shared pointer's IsValid is still true? If IsValid is holding true over the thread's removal from the thread list, then I'm worried that this change will keep us using the old ThreadSP that was reported the next time we stopped and this thread ID was represented by a different ThreadSP.

So, I landed this patch but had to revert it as it broke the build on MSVC (and only MSVC). That was almost a month ago, but I haven't gotten back around to this until now – sorry :|

Ping...

In D67647#1674795, @dblaikie wrote:

Right - I was suggesting that could be changed. Would it be OK to you to change arguments() to return ArrayRef? Or would you rather avoid that to avoid baking in more dependence on that alias violation?

Addressed feedback.

In D67740#1675564, @dblaikie wrote:

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

In D67743#1675533, @dblaikie wrote:

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the design implications of that. I have some doubts about supporting non-consumed after passing by rvalue ref, but don't feel too strongly & the alternative would be having a warning about the attribute being ignored, etc - when it has a fairly clear meaning if it is there, just not sure people 'should' be doing that.

Fixed a mistake where the diagnostic would print the wrong parameter type.

In D67647#1674773, @dblaikie wrote:

I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?

Here's a new version of the patch that uses iterator ranges instead of ArrayRef, to avoid adding new uses of CallExpr::getArgs() in case it has to be removed in the future due to the strict aliasing issue.

Ugh, it looks like getArgs() is a massive strict aliasing violation. And it's used in enough different places in Clang that fixing the violation may require extensive refactoring... I've reported the issue as https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it :)

Sounds good; I'll split this into a few separate patches.

Ping. I'm not qualified to review this myself but I'd like to see the bug fixed. :)

Ping.

Heh, I guess I'll request commit access, although I'm not sure if I have enough of a 'track record of submitting high quality patches'. But for now, can you commit this? Thanks :)

Addressed review comments.

Changes since last version:

Oops, I forgot to re-run git diff after fixing the patch.

Thanks for the feedback, updated.

Oops, forgot to mention:

Thanks for the review. I don't have commit access.

Second ping.

Ping.

Oh, forgot to mention in the initial submission: I don't have commit access so I'll need someone else to commit this.

Fix disassembling ARM instructions as big-endian

Sorry for the delay; here's an updated diff with a test. (The test didn't seem to clearly belong in any existing .s file I saw, so I just made a new one.)

Ping, and adding potential reviewers like I was supposed to do in the first place.

I don't have SVN access. If you want to commit it, feel free to remove that comment, or else I'll update the diff tomorrow.

Addressed comments. Thanks, rsmith!

(Ping? I verified yesterday that the patch still applies and passes regression tests.)

One more ping. As far as I know, everything has been addressed.

Okay.

Fixed raised issues.

Ping again.

Ping?

Per vsk:

• Changed to an explicit switch, and changed ContainsPlaceholderType logic.