Page MenuHomePhabricator

amccarth (Adrian McCarthy)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 26 2015, 9:26 AM (238 w, 4 d)

Recent Activity

Today

amccarth accepted D66633: Breakpad: Add support for parsing STACK WIN records.

LGTM.

Fri, Aug 23, 11:38 AM

Yesterday

amccarth added a comment to D66447: Add char8_t support (C++20).

A couple inline comments. I think this is looking pretty good.

Thu, Aug 22, 2:34 PM · Restricted Project, Restricted Project
amccarth added a comment to D66445: Explicitly Cast Constants to DWORD.

Is this necessary? I see

Thu, Aug 22, 8:47 AM · Restricted Project

Fri, Aug 16

amccarth added inline comments to D66345: [lldb][NFC] Allow for-range iterating over StringList.
Fri, Aug 16, 3:37 PM · Restricted Project

Wed, Aug 14

amccarth resigned from D56010: [NativePDB] Fix setting breakpoint by file and line.
Wed, Aug 14, 4:02 PM
amccarth added a comment to D56010: [NativePDB] Fix setting breakpoint by file and line.

It turns out these tests all seem to exist, so I guess Zachary submitted them as part of other CLs.

Wed, Aug 14, 4:02 PM

Fri, Aug 9

amccarth added a comment to D65230: [CMake] Loosen Python version check and ignore patch version.

Just closing the loop. Regarding the "Python memory allocator called without holding the GIL" bugs: this is not a "just me" problem. It's for anyone who enables debug mode. This appears to be a test infrastructure problem, but it might be harmless. If I run in "release" mode, I get the same behavior as the Windows build bot.

Fri, Aug 9, 4:58 PM · Restricted Project, Restricted Project
amccarth added a comment to D56010: [NativePDB] Fix setting breakpoint by file and line.

It looks like the code changes landed (probably as part of another patch) but not the tests. I'll look and see if the tests should to be revived.

Fri, Aug 9, 3:00 PM

Thu, Aug 8

amccarth accepted D65955: Minidump/Windows: Fix module lookup.

This looks fine, and thanks for the tests for a one-line fix.

Thu, Aug 8, 10:14 AM · Restricted Project

Mon, Aug 5

amccarth added inline comments to D65691: Various build fixes for lldb on MinGW.
Mon, Aug 5, 3:20 PM · Restricted Project, Restricted Project
amccarth added a comment to D65691: Various build fixes for lldb on MinGW.

Yes, zturner isn't as active here as he was before.

Mon, Aug 5, 8:59 AM · Restricted Project, Restricted Project

Thu, Aug 1

amccarth committed rG5f5379d0767d: Fix TestThreadSpecificBreakpoint on Windows (authored by amccarth).
Fix TestThreadSpecificBreakpoint on Windows
Thu, Aug 1, 7:50 AM
amccarth committed rL367573: Fix TestThreadSpecificBreakpoint on Windows.
Fix TestThreadSpecificBreakpoint on Windows
Thu, Aug 1, 7:50 AM
amccarth closed D65546: Fix TestThreadSpecificBreakpoint on Windows.
Thu, Aug 1, 7:50 AM · Restricted Project

Wed, Jul 31

amccarth created D65546: Fix TestThreadSpecificBreakpoint on Windows.
Wed, Jul 31, 3:46 PM · Restricted Project

Tue, Jul 30

amccarth accepted D65437: [lldb][docs] Update landing page for monorepo.
Tue, Jul 30, 8:15 AM · Restricted Project, Restricted Project

Fri, Jul 26

amccarth added inline comments to D65330: [lldb][docs] Update documentation for monorepo and CMake caches.
Fri, Jul 26, 11:09 AM · Restricted Project, Restricted Project
amccarth added a comment to D65330: [lldb][docs] Update documentation for monorepo and CMake caches.

Looks good. Thanks for doing these updates!

Fri, Jul 26, 9:54 AM · Restricted Project, Restricted Project
amccarth added a comment to D65230: [CMake] Loosen Python version check and ignore patch version.

Thanks.

BTW, I tried this on windows, and it didn't blow up in my face, I think it should be relatively safe. I couldn't reproduce the problems that @amccarth was experiencing, so I don't know if it fixes that problem. It would be good to know whether it does, as we could cherry-pick this to the release branch in that case.

Fri, Jul 26, 9:42 AM · Restricted Project, Restricted Project

Jul 22 2019

amccarth committed rG3f0621029505: [Windows] Fix race condition between state changes (authored by amccarth).
[Windows] Fix race condition between state changes
Jul 22 2019, 10:08 AM
amccarth committed rL366703: [Windows] Fix race condition between state changes.
[Windows] Fix race condition between state changes
Jul 22 2019, 10:03 AM
amccarth closed D62183: [Windows] Fix race condition between state changes.
Jul 22 2019, 10:02 AM · Restricted Project, Restricted Project
amccarth added a comment to D62183: [Windows] Fix race condition between state changes.

Yes, I can submit it for you, probably in the next hour or two. Thanks for the patch!

Jul 22 2019, 9:11 AM · Restricted Project, Restricted Project

Jul 18 2019

amccarth accepted D64689: [NativePDB] Make GetOrCreateDeclForUid return an lldb CompilerDecl.

Thanks for the change. I was thinking Expected based on the name of the function: GetOrCreateDeclForUid. I was thinking that it would be odd to have a UID if you don't have debug info. Anyway, Optional is fine. Thanks again.

Jul 18 2019, 8:33 AM
amccarth requested changes to D63165: Initial support for native debugging of x86/x64 Windows processes.
In D63165#1540606, @Hui wrote:

Sorry for the stupid question, but ...

What exactly is meant here by "Native"? How is a NativeProcessWindows different from ProcessWindows?

The Native*** classes are meant to be used from lldb-server. They look somewhat similar to their non-native counterpart because they still do debugging, but they're a lot dumber, because they only deal with basic process control, and none of the fancy symbolic stuff that you'd need debug info for.

They differ in APIs but most of them have common implementations. The APIs from native process classes are more easy to apply process/thread control.
Hope the native and non-native ones can be merged. The similar thing to the RegisterContext and NativeRegisterContext classes.

The other thing is that using "native" classes can avoid linking a lot of unnecessary lldb libs (LLDB plugins or whatever comes with the plugins) to lldb-server.
The nativeprocesswindows could just be a pass-through to processwindows plugin, but the usage is a sort of strange since the
lldb-server needs to initialize the plugin, create a target, and create a instance just like what lldb does. This means literally
there will be two lldb debuggers, one on host and the other one on remote. It is doable, but not that applicable.

So the idea is ProcessWindows will be deleted once lldb-server supports windows debugging. A bit of history here: when we first started we started making ProcessXXXX for each platform (mac, linux, windows). Then we thought about remote debugging and decided to have everyone just make a lldb-server for their platform and even when we are locally debugging, we launch a lldb-server. This is how linux, macOS, the BSDs and other targets do it. Why? Because if you do it the other way you have more code to support: one native plug-in and one remote plug-in. This means the remote debugging usually has more issues since it is the less tested debugging scenario. It also means that if you had a native process implementation only, like ProcessWindows is currently, you can't remotely debug to a windows machine.

So yes there is duplicated code for now, but ProcessWindows.cpp and ThreadWindows.cpp should go away in the near future once lldb-server takes over so the temporary code duplication is just to keep things working until lldb-server takes over.

Jul 18 2019, 8:18 AM · Restricted Project, Restricted Project

Jul 17 2019

amccarth accepted D62183: [Windows] Fix race condition between state changes.
Jul 17 2019, 4:16 PM · Restricted Project, Restricted Project
amccarth committed rG3628a8fae9f3: [NFC] Clarify a Cmake status message regarding Python on LLDBConfig (authored by amccarth).
[NFC] Clarify a Cmake status message regarding Python on LLDBConfig
Jul 17 2019, 3:37 PM
amccarth committed rL366383: [NFC] Clarify a Cmake status message regarding Python on LLDBConfig.
[NFC] Clarify a Cmake status message regarding Python on LLDBConfig
Jul 17 2019, 3:36 PM
amccarth added a comment to D64881: [Cmake] Use the modern way to find Python when possible.

An aside ...

I'm still trying to get back to a buildable state the earlier changes, like the one that tries to enforce version consistency between the libs and the interpreter. I'm currently bisecting to figure out what I hope is the final blocker.

Maybe we should just revert D64443? This is what broke my setup, and led me to introduce to the consistency check in the first place, which merely diagnoses a real problem.

Jul 17 2019, 2:46 PM · Restricted Project
amccarth added a comment to D64881: [Cmake] Use the modern way to find Python when possible.

An aside ...

Jul 17 2019, 2:05 PM · Restricted Project
amccarth added a comment to D62183: [Windows] Fix race condition between state changes.

This change looks fine to me, but I wish there was a reliable way to test it. Is it a true race condition (e.g., because the unpredictability of thread scheduling), or is there a way to write a test that would always fail without this fix?

Jul 17 2019, 11:33 AM · Restricted Project, Restricted Project
amccarth accepted D64851: [NativePDB] Add a FromCompilerDecl for going from lldb -> clang.
Jul 17 2019, 9:09 AM · Restricted Project
amccarth added a comment to D64689: [NativePDB] Make GetOrCreateDeclForUid return an lldb CompilerDecl.

Oh, and thanks for the background and context on the motivation for this change!

Jul 17 2019, 8:12 AM
amccarth accepted D64689: [NativePDB] Make GetOrCreateDeclForUid return an lldb CompilerDecl.

LGTM, but please reconsider whether the return type of GetOrCreateDeclForUid should be changed in this patch rather than in a future one.

Jul 17 2019, 8:11 AM
amccarth added a comment to D64688: [NativePDB] Make GetTranslationUnitDecl return an lldb CompilerDeclCtx.

LGTM.

Jul 17 2019, 8:04 AM

Jul 16 2019

amccarth added a comment to D64812: [CMake] Fail when Python interpreter doesn't match Python libraries version .

OK, the only way I was able to make this work was to remove all traces of Python 2.x from my machine. As long as the older Python existed on my machine CMake would find that one, regardless of which one was in my PATH or indicated by PYTHON_HOME_DIR. Of course, it required killing the cmake cache a couple times, too.

Jul 16 2019, 4:12 PM · Restricted Project, Restricted Project
amccarth added a comment to D64812: [CMake] Fail when Python interpreter doesn't match Python libraries version .
In D64812#1588061, @rnk wrote:
In D64812#1588055, @rnk wrote:

You broke my build. =/ I got this output:

CMake Error at C:/src/llvm-project/lldb/cmake/modules/LLDBConfig.cmake:201 (message):
  Found incompatible Python interpreter (3.7.3) and Python libraries ()

I'll mess with it a bit I guess.

Fixed that in r366247. Apparently Windows has totally custom Python version detection logic.

Jul 16 2019, 3:19 PM · Restricted Project, Restricted Project
amccarth added inline comments to D64824: [CMake] Move standalone check so we don't have to reconfigure LLDB .
Jul 16 2019, 3:07 PM · Restricted Project, Restricted Project
amccarth added a comment to D64821: [CMake] Remove duplicated logic to find Python when doing a standalone build.

Actually, right now I'm trying to figure out where the interpreter is found because cmake is finding different, incompatible versions of the interpreter (2.7) and the libs (3.6).

Jul 16 2019, 3:04 PM · Restricted Project, Restricted Project
amccarth added inline comments to D64688: [NativePDB] Make GetTranslationUnitDecl return an lldb CompilerDeclCtx.
Jul 16 2019, 11:01 AM
amccarth added a comment to D64689: [NativePDB] Make GetOrCreateDeclForUid return an lldb CompilerDecl.

Was there a proposal or discussion (e.g., a thread on lldb-dev) about making PdbAstBuilder abstract in order to handle other languages? It sounds like a good idea, but I'd like to catch up on any context I may have missed.

Jul 16 2019, 10:38 AM

Jul 9 2019

amccarth added a comment to D64444: Add Python 3.6 and 3.7 to the version list.

Be aware that Swig 3.0.12 has a compatibility problem with Python 3.7 that causes hundreds of LLDB tests to fail an assertion (at least on Windows).

Jul 9 2019, 1:52 PM · Restricted Project, Restricted Project

Jun 25 2019

amccarth committed rG17c18a9e8161: Fix a typo in help text. (authored by amccarth).
Fix a typo in help text.
Jun 25 2019, 4:14 PM
amccarth committed rL364361: Fix a typo in help text..
Fix a typo in help text.
Jun 25 2019, 4:14 PM
amccarth added a comment to D63790: [dotest] Add the ability to set environment variables for the inferior..

The LGTM, but I wasn't nominated as a reviewer, and I was mostly looking at it from the point of Windows compatibility.

Jun 25 2019, 4:04 PM · Restricted Project, Restricted Project
amccarth added inline comments to D63790: [dotest] Add the ability to set environment variables for the inferior..
Jun 25 2019, 3:29 PM · Restricted Project, Restricted Project

Jun 24 2019

amccarth added a comment to D62183: [Windows] Fix race condition between state changes.

zturner is not a regular here anymore. I think he pops in from time-to-time, but I wouldn't depend on just him for a review.

Jun 24 2019, 4:45 PM · Restricted Project, Restricted Project

Jun 18 2019

amccarth committed rG46e6e1329875: Fix some lit test ResourceWarnings on Windows (authored by amccarth).
Fix some lit test ResourceWarnings on Windows
Jun 18 2019, 9:36 AM
amccarth committed rL363700: Fix some lit test ResourceWarnings on Windows.
Fix some lit test ResourceWarnings on Windows
Jun 18 2019, 9:34 AM
amccarth closed D63102: Fix some lit test ResourceWarnings on Windows.
Jun 18 2019, 9:34 AM · Restricted Project

Jun 17 2019

amccarth added inline comments to D63102: Fix some lit test ResourceWarnings on Windows.
Jun 17 2019, 4:17 PM · Restricted Project

Jun 14 2019

amccarth added a comment to D63166: Move common functionality from processwindows into processdebugger.

I'm OK with moving common stuff out for now, and I like the separation of ProcessWindows and ProcessDebugger.

Jun 14 2019, 12:03 PM · Restricted Project

Jun 13 2019

amccarth added a comment to D63166: Move common functionality from processwindows into processdebugger.

It's been a while for me, so I'm not super-familiar with the code being changed, but I'm okay with factoring out common code. I agree with labath's open points and will try to look at it in more detail tomorrow.

Jun 13 2019, 4:31 PM · Restricted Project

Jun 11 2019

amccarth added a comment to D63165: Initial support for native debugging of x86/x64 Windows processes.

Sorry for the stupid question, but ...

Jun 11 2019, 4:36 PM · Restricted Project, Restricted Project

Jun 10 2019

amccarth created D63102: Fix some lit test ResourceWarnings on Windows.
Jun 10 2019, 3:48 PM · Restricted Project

Jun 7 2019

amccarth committed rG4ca8435528ca: Fix string literals to avoid deprecation warnings in regexp patterns (authored by amccarth).
Fix string literals to avoid deprecation warnings in regexp patterns
Jun 7 2019, 2:13 PM
amccarth committed rGa4198c22dc1d: NFC: Fix typo in a cmake message (authored by amccarth).
NFC: Fix typo in a cmake message
Jun 7 2019, 2:13 PM
amccarth committed rL362846: Fix string literals to avoid deprecation warnings in regexp patterns.
Fix string literals to avoid deprecation warnings in regexp patterns
Jun 7 2019, 2:12 PM
amccarth closed D62882: Use raw strings to avoid deprecation warnings in regexp patterns.
Jun 7 2019, 2:12 PM · Restricted Project
amccarth committed rG4447d15aef08: Fix lit tests on Windows related to CR+LF (authored by amccarth).
Fix lit tests on Windows related to CR+LF
Jun 7 2019, 2:11 PM
amccarth committed rL362845: NFC: Fix typo in a cmake message.
NFC: Fix typo in a cmake message
Jun 7 2019, 2:11 PM
amccarth committed rL362844: Fix lit tests on Windows related to CR+LF.
Fix lit tests on Windows related to CR+LF
Jun 7 2019, 2:10 PM
amccarth closed D62759: Fix lit tests on Windows related to CR.
Jun 7 2019, 2:10 PM · Restricted Project
amccarth added inline comments to D62882: Use raw strings to avoid deprecation warnings in regexp patterns.
Jun 7 2019, 2:09 PM · Restricted Project
amccarth updated the diff for D62882: Use raw strings to avoid deprecation warnings in regexp patterns.

Changed the approach in the first fix to use explicit escaping after Joel Denny's comment got me to re-think it.

Jun 7 2019, 1:36 PM · Restricted Project
amccarth added a comment to D62882: Use raw strings to avoid deprecation warnings in regexp patterns.

But how do I see the bad behavior or deprecation warnings? Python 3.6.7 with -ddd didn't reveal anything on a simple example I just tried.

Jun 7 2019, 10:49 AM · Restricted Project

Jun 4 2019

amccarth created D62882: Use raw strings to avoid deprecation warnings in regexp patterns.
Jun 4 2019, 3:53 PM · Restricted Project
amccarth added a reviewer for D62759: Fix lit tests on Windows related to CR: stella.stamenova.
Jun 4 2019, 3:38 PM · Restricted Project

May 31 2019

amccarth created D62759: Fix lit tests on Windows related to CR.
May 31 2019, 4:19 PM · Restricted Project

May 22 2019

amccarth accepted D60962: [NativePDB] Extend .pdb files search folders.
May 22 2019, 3:30 PM · Restricted Project

May 9 2019

amccarth accepted D61733: Breakpad: Generate unwind plans from STACK CFI records.

LGTM once you double-check the return value in the error case at the end of SymbolFileBreakpad::ParseUnwindRow.

May 9 2019, 1:05 PM · Restricted Project

May 8 2019

amccarth added inline comments to D61686: Enable lldb-server on Windows.
May 8 2019, 12:48 PM · Restricted Project
amccarth added inline comments to D61686: Enable lldb-server on Windows.
May 8 2019, 11:33 AM · Restricted Project

Apr 30 2019

amccarth accepted D61311: PostfixExpression: Use signed integers in IntegerNode.

This looks fine to me, and the rationale sounds right. I'll be curious to see if any other reviewers see a potential problem with this.

Apr 30 2019, 7:47 AM · Restricted Project

Apr 29 2019

amccarth accepted D61183: PostfixExpression: Introduce CFANode.

LGTM, but I found one comment a bit confusing for me.

Apr 29 2019, 1:01 PM · Restricted Project
amccarth accepted D61128: Support member function types in PdbAstBuilder.

Thanks for the improved commit message. Again, sorry about the delay.

Apr 29 2019, 11:26 AM · Restricted Project

Apr 25 2019

amccarth added a comment to D61128: Support member function types in PdbAstBuilder.

Please add some detail to the commit message and consider re-titling it. It looks like this patch is actually adding missing functionality rather than "fixing" a bug in how structs are handled. If that's correct, then please make the commit message reflect that.

Apr 25 2019, 5:18 PM · Restricted Project
amccarth accepted D61056: PostfixExpression: move DWARF generator out of NativePDB internals.

Clever (hopefully not too clever)! Not having to derive a special class from Visitor is really nice.

Apr 25 2019, 11:04 AM · Restricted Project

Apr 24 2019

amccarth added a comment to D61056: PostfixExpression: move DWARF generator out of NativePDB internals.

Can the test deletions be a separate patch, or will they fail because of the other changes in this patch? They feel like a good but separable step.

Apr 24 2019, 4:38 PM · Restricted Project
amccarth added a comment to D60962: [NativePDB] Extend .pdb files search folders.

Thanks Pavel!

Apr 24 2019, 9:15 AM · Restricted Project

Apr 23 2019

amccarth accepted D61003: PostfixExpression: move parser out of NativePDB internals.

Nice. Thanks for adding the test, too!

Apr 23 2019, 3:48 PM · Restricted Project
amccarth added a comment to D60962: [NativePDB] Extend .pdb files search folders.

cc: Pavel Labath because I know he's been involved in conversations about how to find symbol files in general (PDB, split DWARF, Breakpad, etc.), especially when doing post-mortem debugging.

Apr 23 2019, 3:39 PM · Restricted Project
amccarth updated subscribers of D60962: [NativePDB] Extend .pdb files search folders.
Apr 23 2019, 3:30 PM · Restricted Project
amccarth accepted D61000: Kill modify-python-lldb.py.
Apr 23 2019, 3:26 PM · Restricted Project

Apr 19 2019

amccarth accepted D60817: [NativePDB] Add anonymous namespaces support.

Thanks for the changes. If the tests pass, then this LGTM. Just check the one last question I added about the AC1D test.

Apr 19 2019, 8:18 AM · Restricted Project, Restricted Project

Apr 18 2019

amccarth added inline comments to D60817: [NativePDB] Add anonymous namespaces support.
Apr 18 2019, 1:39 PM · Restricted Project, Restricted Project
amccarth added a comment to D60817: [NativePDB] Add anonymous namespaces support.

Sorry for the slow response; I'm still learning about this code.

Apr 18 2019, 1:35 PM · Restricted Project, Restricted Project

Apr 17 2019

amccarth added a comment to D60519: [Windows] Dump more information about access violation exception.

Thanks for the new test and the test fix. It's unfortunate that the tests are so sensitive to an arbitrary buffer size.

Apr 17 2019, 3:31 PM · Restricted Project, Restricted Project
amccarth accepted D60405: MinidumpYAML: Add support for ModuleList stream.

Nice!

Apr 17 2019, 3:23 PM · Restricted Project
amccarth accepted D60667: Allow direct comparison of ConstString against StringRef.

I, too, have some concern that this could have unintended side effects. To make the temporary StringRefs from the zero-terminated strings requires a strlen call each time. So we're making two passes over a string each time (once to measure and once to compare). Granted, these are mostly very short.

I think I'm not understanding something here. There is one strlen call to make the StringRef from the literal, but I don't see why we need a second one to compare (as StringRef and ConstString both store the string length and don't recompute it when comparing). Simple godbolt test case here: https://godbolt.org/z/9X1RRt So both the old and new version both require one strlen call from what I can see (which is anyway optimized out for comparing against literals).

Apr 17 2019, 8:36 AM · Restricted Project, Restricted Project

Apr 15 2019

amccarth added a comment to D60667: Allow direct comparison of ConstString against StringRef.

I, too, have some concern that this could have unintended side effects. To make the temporary StringRefs from the zero-terminated strings requires a strlen call each time. So we're making two passes over a string each time (once to measure and once to compare). Granted, these are mostly very short.

Apr 15 2019, 1:24 PM · Restricted Project, Restricted Project

Apr 11 2019

amccarth accepted D60410: PDBFPO: Improvements to the AST visitor.

I'm even happier. Thanks for giving the params more specific names.

Apr 11 2019, 1:18 PM · Restricted Project

Apr 10 2019

amccarth accepted D60501: Minidump: extend UUID byte-swapping to windows platform.
Apr 10 2019, 4:54 PM · Restricted Project
amccarth added a comment to D60519: [Windows] Dump more information about access violation exception.

Does this affect any existing tests? Is there a good way to test it?

Apr 10 2019, 10:53 AM · Restricted Project, Restricted Project
amccarth accepted D60498: Clean up docstrings in swig interface files.
Apr 10 2019, 10:29 AM · Restricted Project
amccarth added a comment to D60498: Clean up docstrings in swig interface files.

LGTM, though I'm not clear why this extra visual noise was there in the first place. Does doxygen depend on this style when reading the swigged API?

Apr 10 2019, 9:51 AM · Restricted Project

Apr 8 2019

amccarth accepted D60410: PDBFPO: Improvements to the AST visitor.

Overall, I'm ambivalent.

Apr 8 2019, 4:25 PM · Restricted Project

Apr 5 2019

amccarth added a comment to D60268: Breakpad: Parse Stack CFI records.

My concerns were address, so LGTM. I'll leave the rest to you and clayborg.

Apr 5 2019, 7:49 AM · Restricted Project

Apr 4 2019

amccarth accepted D60271: PDBFPO: Use references instead of pointers, where possible.

I noticed this also deleted two overloads of Visit from FPOProgramASTVisitorDWARFCodegen, but that appears to be harmless (the base class overloads were also no-ops).

Apr 4 2019, 9:42 AM · Restricted Project
amccarth added a comment to D60268: Breakpad: Parse Stack CFI records.

This looks good. I have a few questions inline, but none of those are major concerns.

Apr 4 2019, 9:23 AM · Restricted Project