Address feedback from many.

In D63745#1556941, @labath wrote:

This code should go to tools/debugserver/source/CMakeLists.txt so that it is next to the code which performs the actual code signing. Doing that will make it easier to keep it in sync with changes to code signing, as well as make it obvious that it is not in sync with them right now (there's a pretty complex interaction of various cmake options (LLDB_CODESIGN_IDENTITY, LLVM_CODESIGNING_IDENTITY, LLDB_USE_SYSTEM_DEBUGSERVER, etc.) which affects code signing, and this code is ignoring all of those)...

rdar://problem/52078735

LGTM.

In D63488#1548991, @probinson wrote:

I see the point; certainly when someone emails a patch, the first response is almost always "can you put this on Phab."

There was talk at some point about connecting Phab with github authentication, somehow? If people can use an existing account then the you-need-to-register-first objection goes away.

LGTM!

Can you add a screenshot of the final result for everybody?

yes.

To elaborate further, I'm not familiar with this code so I really shouldn't be the person to approve the patch. On the other hand I'm somewhat nervous as refactors in this area of the codebase caused bugs/crashes in lldb, fundamentally due to changes in behavior previously left untested.

I don't think I'm qualified enough to comment further on this patch.

Do you have a testcase?

LGTM

What if ~ is not writable either? I think we shouldn't fallback.

LGTM, also discussed in person.

LGTM

What's the scenario that's causing this? Adding a nullptr check is an obviously save thing to do, but it would be excellent if we could add a comment explaining why the symbol could be nullptr.
I also do believe that the logic for checking whether the symbol is nullptr can be hoisted to the beginning of the function, see comment inline.

LGTM

LGTM

yay :)

LGTM

LGTM but this should really be fixed upstream (in libedit)

While the intentions of exposing this are good, the implementation leaks too many details. In fact, there shouldn't be anything outside of ScriptInterpreter guarded by LLDB_DISABLE_PYTHON.
You might want also to realize that having ePathTypePythonDir goes against the whole notion of a generic pluggable interpreter.
Imagine tomorrow somebody wants to implement support for another language, what would this call return for it?

@clayborg I would also like to point out that some of us fair amount of time cleaning LLDB_DISABLE_PYTHON leaking all over the codebase, and this is basically the last occurrence, so we're motivated to have this going away/fixed in some way.

Jonas' comments

updated.

OK, on it.

Add an unittest maybe? otherwise, seems to match the ISA so LGTM

I'm not necessarily sure I like this. It's simply spreading in a general framework a concept that shouldn't exist to begin with. I'd rather remove forced constant from SCCP, even though it might result in us losing some optimization power.

In D60300#1456472, @clayborg wrote:

In D60300#1456468, @JDevlieghere wrote:

In D60300#1455790, @davide wrote:

I think this is good regardless for readability, but I would really appreciate if we can collect some numbers to see how effective this change actually is.

I see 120 -> 12 seconds on my iMac Pro.

Nice! Might be nice to have some automated info that gets dumped when one file with multiple tests takes a long time and suggest breaking them up in the python test infrastructure modules. Might be other nice gains to be had?

In D60300#1456491, @davide wrote:

In D60300#1456472, @clayborg wrote:

In D60300#1456468, @JDevlieghere wrote:

In D60300#1455790, @davide wrote:

I think this is good regardless for readability, but I would really appreciate if we can collect some numbers to see how effective this change actually is.

I see 120 -> 12 seconds on my iMac Pro.

Nice! Might be nice to have some automated info that gets dumped when one file with multiple tests takes a long time and suggest breaking them up in the python test infrastructure modules. Might be other nice gains to be had?

lit has this feature. It's called --time-tests. That's how we discovered the problem. There are other tests that we can fix.

--Davide

great!

I think this is good regardless for readability, but I would really appreciate if we can collect some numbers to see how effective this change actually is.

Thanks.

LGTM. Med, what do you think?

LGTM

LGTM

In D59911#1446787, @aprantl wrote:

Thanks for summarizing your thoughts, Davide.

I think that what you wrote is a much better explanation of what I was trying to say with

Use these sparingly and only if error handling is not otherwise feasible.

I think that unless we can eliminate all uses of lldb_assert() from the code we should document it, but discourage its use. Do you have any suggestions for a better wording?