This also doesn't work on my macOS system:

FAIL: LLDB (/Users/teemperor/1llvm/rel/bin/clang-x86_64) :: test_stop_hooks_scripted_return_false (TestStopHookScripted.TestStopHooks)
======================================================================
FAIL: test_stop_hooks_scripted_return_false (TestStopHookScripted.TestStopHooks)
   Test that the returning False from a stop hook works
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/teemperor/1llvm/llvm-project/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py", line 54, in test_stop_hooks_scripted_return_false
    self.do_test_auto_continue(True)
  File "/Users/teemperor/1llvm/llvm-project/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py", line 91, in do_test_auto_continue
    self.assertEqual("main", func_name, "Didn't stop at the expected function.")
AssertionError: 'main' != 'step_out_of_me'
- main+ step_out_of_me : Didn't stop at the expected function.
Config=x86_64-/Users/teemperor/1llvm/rel/bin/clang
----------------------------------------------------------------------

Done. Thanks for the patch!

To clarify the description a bit: RegInfoBasedABI::GetRegisterInfoByName is comparing the name/alt_name by doing a const char * pointer value comparison. That only works if both strings that are compared are coming from a ConstString. Since b0060c3a7868ef026d95d0cf8a076791ef74f474 GetRegisterInfoByName is checking with an assert that both C strings came from a ConstString (which is failing as these ABI implementation changed here are lacking the ConstString'ification code).

If anyone knows another SB object where calling str on it would execute a Python script (and that is simpler than an synthetic child provider), then let me know.

@MaskRay is this ready to land? (This is technically in the 'accepted' state so this review probably disappeared from everyone's review queue, hence my ping)

• Don't use a typedef in the formatter test case (which already has a special code path) but just a struct.

In D87619#2273450, @martong wrote:

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

• Removed an unrelated change.

In D87481#2270919, @jarin wrote:

This makes sense, thanks!

Since your main motivation is formatter selection, you might want to consider adding a formatting test, too. To be precise, I see that you have a formatting test for typedef already, but that one would pass even without your change (as far as I can see). I mean a formatting test for the case described in your patch summary, that one should fail without your patch.

• Added early-exit in CallExpr comparison.
• Added an unreachable to fix a missing return warning.

Thanks for the quick review!

• Remove FunctionDecl body comparison workaround.
• Add makeStmt and makeWrappedStmt with AST matcher parameters.

In D87327#2263971, @kastiglione wrote:

@teemperor I'm not sure what these build errors mean, for example https://reviews.llvm.org/B70998. Also there's the linux build failure, but that's not relevant for debugserver, right?

Thanks!

• Clarify that this isn't just a direct revert, but reverts by establishing a set again instead of just a plain vector (Thanks Shafik!)

In D85648#2216345, @shafik wrote:

LGTM, it is sad we lose the deterministic ordering though. Would it be efficient enough to use a vector but keep the set to prevent duplicates?

LGTM.

LGTM, thanks for the cleanup! (And also having the second list of comments no longer always stating 'SpringBoard' is even better).

• Upload correct diff.

• Retry uploading diff.

• Added comments to unimplemented functions.
• Changed repl source file name.

(And I'm aware there are a lot more tests needed to test all the REPL code that had no tests so far. I'll see what's missing once this his the coverage bot and then I'll make put reviews up for that).

In D86344#2260056, @chandlerc wrote:

Somewhat minor post-commit thought, but we now have both a library FileCheck and an executable binary FileCheck (not to mention the existing confusion of having two FileCheck.cpp files).

Might be nice to think about a naming scheme that would reduce these collisions?

• Fix typo and argument comments (thanks MaskRay !)

• Removed driver arg and now tested via a unit test.

In D80878#2258942, @riccibruno wrote:

And what if deserialization is forced?

In D85141#2191229, @shafik wrote:

Do we understand how to fix the crash in the case where we get a pair back? I am not sure how frequent the use case is.

In D83038#2256974, @MaskRay wrote:

Isn't DiagOpts->ShowLine sufficient? I don't think clang command line users will specify -fno-show-line...

I have to agree with @kastiglione that this isn't very intuitive. I doubt that more than a handful of users are aware that "raw commands" are a thing, so I think this inconsistency is for once in favor of the user. LLDB commands are anyway not meant to be stable, so we can just fix this if this ever turns out to be a real issue.

• Fixed formatting in switch-case
• Fixed formatting in DiagnosticOptions.def

Sorry, I totally didn't see this patch (I also only added Fred as a reviewer as he added the assert, so there weren't even any shared reviewers who could have pointed this out).

• rename to maybe_constexpr
• Update some dump output tests

This is breaking the Green Dragon bot: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/23667/console

This patch sparks joy, but the fact that this all is dead code without Swift doesn't spark joy. But as it's up to me to revive this code and the downstream Swift test looks fine, I'll sign this off. Thanks!

In D87036#2254108, @labath wrote:

Looks good.

In D87036#2252346, @teemperor wrote:

This should also avoid changing the test all the time when someone touches one of the symbols (as in D68533 )

AFAICT, that patch modified the test because it actually introduced a new symbol -- the order of the existing symbols was not changed.

In D85993#2253465, @amccarth wrote:

In D85993#2220724, @labath wrote:

Dwarf parser uses TypeSystemClang::AddMethodToCXXRecordType instead of this function to create methods (which is why there are no assertions like this when using dwarf). Maybe it would be better to change the pdb parser to use that function instead (as it allows the user to specify not only accessibility, but also other potentially useful properties of the created method -- static, virtual, etc.).

The NativePDB AST parser also uses AddMethodToCXXRecordType. It's in udtcompleter.cpp.

But the bug is triggered while the plugin is trying to synthesize the function declaration during a ResolveSymbolContext call. It looks like the DWARF implementation of ResolveSymbolContext relies on its AST parser, but the NativePDB one does not. I'm trying to figure out how to do that.

Also note that AddMethodToCXXRecordType just sets whatever access type it's asked to set. Before calling AddMethodToCXXRecordType, the DWARF parser has this gem:

// Neither GCC 4.2 nor clang++ currently set a valid
// accessibility in the DWARF for C++ methods...
// Default to public for now...
if (attrs.accessibility == eAccessNone)
  attrs.accessibility = eAccessPublic;

That bit of logic is what prevents the assertion from failing for DWARF. I'll can make the NativePDB AST parser do the same thing.

But that won't fix the bug, since the NativePDB AST parser isn't involved (at least, my breakpoints in the parser never hit during the course of the test). In the mean time, I'll try to figure out how to get the NativePDB plugin's ResolveSymbolContext implementation to use AddMethodToCXXRecord.

Just to also jump in on the thread: This broke the modules builds. I fixed that in https://reviews.llvm.org/rGe0e7eb2e2648aee83caf2ecfe2972ce2f653d306 so please recommit that patch or merge it into this revision before relanding. Thanks!

LGTM

• Make the assert_completions_contains remove an element after it was found (to prevent finding the same element twice).
• Removed special assert for negative return codes.

This should also avoid changing the test all the time when someone touches one of the symbols (as in D68533 )