Page MenuHomePhabricator

amccarth (Adrian McCarthy)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

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.

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

Thu, Apr 18

amccarth added inline comments to D60817: [NativePDB] Add anonymous namespaces support.
Thu, Apr 18, 1:39 PM · 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.

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

Wed, Apr 17

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.

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

Nice!

Wed, Apr 17, 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).

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

Mon, Apr 15

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.

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

Thu, Apr 11

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

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

Thu, Apr 11, 1:18 PM · Restricted Project

Wed, Apr 10

amccarth accepted D60501: Minidump: extend UUID byte-swapping to windows platform.
Wed, Apr 10, 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?

Wed, Apr 10, 10:53 AM · Restricted Project
amccarth accepted D60498: Clean up docstrings in swig interface files.
Wed, Apr 10, 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?

Wed, Apr 10, 9:51 AM · Restricted Project

Mon, Apr 8

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

Overall, I'm ambivalent.

Mon, Apr 8, 4:25 PM · Restricted Project

Fri, Apr 5

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.

Fri, Apr 5, 7:49 AM · Restricted Project

Thu, Apr 4

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).

Thu, Apr 4, 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.

Thu, Apr 4, 9:23 AM · Restricted Project

Wed, Apr 3

amccarth committed rGffa857c7a655: Fix and simplify PrepareCommandsForSourcing (authored by amccarth).
Fix and simplify PrepareCommandsForSourcing
Wed, Apr 3, 12:48 PM
amccarth committed rLLDB357626: Fix and simplify PrepareCommandsForSourcing.
Fix and simplify PrepareCommandsForSourcing
Wed, Apr 3, 12:48 PM
amccarth committed rL357626: Fix and simplify PrepareCommandsForSourcing.
Fix and simplify PrepareCommandsForSourcing
Wed, Apr 3, 12:48 PM
amccarth closed D60152: Fix and simplify PrepareCommandsForSourcing.
Wed, Apr 3, 12:48 PM · Restricted Project
amccarth accepted D60195: modify-python-lldb.py: (Re)move __len__ and __iter__ support.

Nice! Thanks for cleaning up the affected comments as well.

Wed, Apr 3, 9:11 AM · Restricted Project
amccarth updated the diff for D60152: Fix and simplify PrepareCommandsForSourcing.

Further simplification per labath's feedback.

Wed, Apr 3, 9:08 AM · Restricted Project
amccarth added a comment to D60152: Fix and simplify PrepareCommandsForSourcing.

Thanks for the review.

Wed, Apr 3, 8:49 AM · Restricted Project

Tue, Apr 2

amccarth accepted D60119: modify-python-lldb.py: clean up __iter__ and __len__ support.
Tue, Apr 2, 4:50 PM · Restricted Project
amccarth added reviewers for D60152: Fix and simplify PrepareCommandsForSourcing: zturner, labath, rnk.
Tue, Apr 2, 2:15 PM · Restricted Project
amccarth created D60152: Fix and simplify PrepareCommandsForSourcing.
Tue, Apr 2, 2:15 PM · Restricted Project

Mon, Apr 1

amccarth accepted D60068: PDBFPO: Refactor register reference resolution.
Mon, Apr 1, 2:51 PM · Restricted Project
amccarth added inline comments to D60094: [MSVC] If unable to find link.exe from a MSVC installation, look for link.exe next to cl.exe.
Mon, Apr 1, 2:24 PM · Restricted Project

Tue, Mar 26

amccarth added a comment to D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary.

The Windows code looks correct to me, but I've added a couple inline questions/comments.

Tue, Mar 26, 5:01 PM · Restricted Project

Mar 12 2019

amccarth committed rG38d4a6c496e3: Correcting some comments in PdbIndex.cpp [NFC] (authored by amccarth).
Correcting some comments in PdbIndex.cpp [NFC]
Mar 12 2019, 10:40 AM
amccarth committed rL355943: Correcting some comments in PdbIndex.cpp [NFC].
Correcting some comments in PdbIndex.cpp [NFC]
Mar 12 2019, 10:40 AM
amccarth committed rLLDB355943: Correcting some comments in PdbIndex.cpp [NFC].
Correcting some comments in PdbIndex.cpp [NFC]
Mar 12 2019, 10:40 AM

Mar 6 2019

amccarth accepted D58975: Introduce MinidumpEnums.def textual header.
Mar 6 2019, 11:44 AM

Feb 28 2019

amccarth closed D58648: Improve process launch comments for Windows.

Closed with r355121.

Feb 28 2019, 11:16 AM
amccarth committed rG34f2bee0fb04: Improve process launch comments for Windows (authored by amccarth).
Improve process launch comments for Windows
Feb 28 2019, 11:16 AM
amccarth committed rL355121: Improve process launch comments for Windows.
Improve process launch comments for Windows
Feb 28 2019, 11:15 AM
amccarth committed rLLDB355121: Improve process launch comments for Windows.
Improve process launch comments for Windows
Feb 28 2019, 11:15 AM
amccarth added a comment to D58648: Improve process launch comments for Windows.

Thanks. I really don't know what other types of conditions may trigger that "request is not supported" message, so I'm going to shy away from trying to make it more specific.

Feb 28 2019, 10:37 AM

Feb 27 2019

amccarth added reviewers for D58648: Improve process launch comments for Windows: labath, lemo.
Feb 27 2019, 3:35 PM

Feb 25 2019

amccarth updated the diff for D58648: Improve process launch comments for Windows.

Forgot to include context in original diff.

Feb 25 2019, 3:21 PM
amccarth created D58648: Improve process launch comments for Windows.
Feb 25 2019, 3:15 PM

Feb 11 2019

amccarth added a comment to D56537: ObjectFilePECOFF: Create a "container" section spanning the entire module image.

My knowledge of PECOFF is even more limited, but it's my understanding that the ImageBase is the _preferred_ address for the module. That doesn't guarantee that's the actual address it would be loaded at. Not just because of ASLR but also because there may be conflicts with modules that have already been loaded. Is GetBaseAddress supposed to return the actual base address or the preferred one?

Feb 11 2019, 9:21 AM · Restricted Project, Restricted Project

Feb 8 2019

amccarth accepted D57895: Breakpad: auto-detect path style of file entries.

I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.

Feb 8 2019, 8:06 AM · Restricted Project

Feb 7 2019

amccarth added inline comments to D57895: Breakpad: auto-detect path style of file entries.
Feb 7 2019, 2:52 PM · Restricted Project

Feb 5 2019

amccarth accepted D57751: minidump: Add ability to attach (breakpad) symbol files to placeholder modules.

Nice.

Feb 5 2019, 11:09 AM · Restricted Project

Jan 10 2019

amccarth added inline comments to D56461: [NativePDB] Add support for parsing typedefs and make lldb-test work with the native reader..
Jan 10 2019, 4:59 PM

Jan 9 2019

amccarth added a comment to D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit.

So this Patch is effectively NFC, since no caller (not even a test) was using the functionality you've removed. Seems like a nice refactor.

Jan 9 2019, 9:10 AM

Jan 8 2019

amccarth added inline comments to D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext.
Jan 8 2019, 2:32 PM

Dec 6 2018

amccarth added a comment to D55344: Support skewed Stream Arrays.

If there is a magic value, should we be checking it?

Dec 6 2018, 11:38 AM
amccarth accepted D55356: Add a method to get the "base" file address of an object file.

LGTM.

Dec 6 2018, 11:05 AM

Nov 30 2018

amccarth added a comment to D55142: Minidump debugging using the native PDB reader.

This looks good to me, save for a couple comment nits.

Nov 30 2018, 4:31 PM · Restricted Project

Jul 19 2018

amccarth added inline comments to D49552: Add a Microsoft Demangler library and utility..
Jul 19 2018, 10:22 AM

Jul 12 2018

amccarth accepted D49202: Restructure the minidump loading path and add early & explicit consistency checks.

LGTM if you don't want to consider my remaining suggestion for this patch.

Jul 12 2018, 8:37 AM · Restricted Project

Jul 11 2018

amccarth added inline comments to D49202: Restructure the minidump loading path and add early & explicit consistency checks.
Jul 11 2018, 3:23 PM · Restricted Project
amccarth added a comment to D49202: Restructure the minidump loading path and add early & explicit consistency checks.

Also, I'm not seeing tests for the other consistency checks you're adding (like whether there are any streams or whether the streams overlap). Is it difficult to create such malformed minidumps?

Jul 11 2018, 3:22 PM · Restricted Project
amccarth added inline comments to D49202: Restructure the minidump loading path and add early & explicit consistency checks.
Jul 11 2018, 2:18 PM · Restricted Project
amccarth added inline comments to D48271: [llvm-readobj] Fix printing format.
Jul 11 2018, 8:41 AM

Jul 10 2018

amccarth added inline comments to D48271: [llvm-readobj] Fix printing format.
Jul 10 2018, 11:00 AM

Jul 9 2018

amccarth added inline comments to D48960: Use an unwinder to get register contexts of frames other than zeroth under Windows.
Jul 9 2018, 4:18 PM · Restricted Project

Jul 3 2018

amccarth added a comment to D48859: Make WindowsSupport.h a public header.

Will making it public cause more TUs to transitively include <windows.h>, or does that happen anyway?

Jul 3 2018, 8:02 AM

Jun 22 2018

amccarth added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

I mostly agree with Zach. In practice, setting core.autocrlf to false on Windows seems the most practical solution.

Jun 22 2018, 11:26 AM

Jun 13 2018

amccarth added a comment to D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails.

My motivation was not to check for different kinds of errors but to ensure that the error code detail doesn't get lost through the SWIG plumbing. SBErrors seem to be marshaled properly when used as return values, but this is the first one I've seen returned by reference. The test ensures that the success/fail result makes it, but not that the detail (e.g., the error string) survives the trip.

Jun 13 2018, 10:55 AM
amccarth added inline comments to D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails.
Jun 13 2018, 9:59 AM

Jun 4 2018

amccarth accepted D47688: [Support] Add functions that return native file handles on Windows instead of fds..
Jun 4 2018, 9:40 AM
amccarth added a comment to D47688: [Support] Add functions that return native file handles on Windows instead of fds..

I like the direction. Just a question about the implicit type conversion.

Jun 4 2018, 8:25 AM

May 31 2018

amccarth added a comment to D47578: Do not enforce absolute path argv0 in windows.
In D47578#1117874, @rnk wrote:

The LLDB test suite isn't in very good shape on Windows. It is complicated to set up and build, I don't want to block this fix on @takuto.ikuta setting up that build environment. This is a Windows-only change, and I believe it makes it more consistent with Linux, so as long as check-llvm, check-clang, and check-lld pass, this should be relatively safe.

May 31 2018, 10:34 AM
amccarth added a comment to D47578: Do not enforce absolute path argv0 in windows.

I was under the impression that some tools rely on the fact that arg[0] is always expanded to an absolute path. Does this work with lldb and its test suite?

May 31 2018, 8:20 AM

May 15 2018

amccarth accepted D46875: [llvm-rc] Add support for the optional CLASS statement for dialogs.
May 15 2018, 11:22 AM
amccarth added inline comments to D46875: [llvm-rc] Add support for the optional CLASS statement for dialogs.
May 15 2018, 8:57 AM

May 14 2018

amccarth accepted D46816: [llvm-rc] Read the Planes/BitCount fields from BITMAPINFOHEADER for icons.
May 14 2018, 2:48 PM
amccarth added inline comments to D46816: [llvm-rc] Read the Planes/BitCount fields from BITMAPINFOHEADER for icons.
May 14 2018, 2:39 PM
amccarth accepted D46818: [llvm-rc] Add support for parsing memory flags.

Thanks for splitting the test.

May 14 2018, 2:33 PM
amccarth added a comment to D46818: [llvm-rc] Add support for parsing memory flags.

I kinda wish the test was broken up a bit. The stringtable bundle logic is distinctly different than just applying the keywords to other resource types, so that would be a natural place to break this apart.

May 14 2018, 9:22 AM
amccarth added inline comments to D46816: [llvm-rc] Read the Planes/BitCount fields from BITMAPINFOHEADER for icons.
May 14 2018, 8:51 AM
amccarth accepted D46813: [llvm-rc] Add missing inputs for tag-icon-cursor.test..

Ah, I see you figured out the discrepancy in a follow-on patch (46816), so this LGTM.

May 14 2018, 8:35 AM
amccarth added inline comments to D46813: [llvm-rc] Add missing inputs for tag-icon-cursor.test..
May 14 2018, 8:28 AM

May 9 2018

amccarth accepted D46506: [llvm-rc] Allow -1 for control IDs in old style dialogs with 16 bit fields .

LGTM, but consider inline suggestion.

May 9 2018, 9:06 AM
amccarth accepted D46636: [llvm-rc] Add support for the RCDATA resource type.

The docs suggest there are differences between RCDATA and user-defined resource definitions, but the Microsoft rc.exe doesn't seem to follow the docs. For example, the docs say user-defined types can use an external file but that RCDATA cannot. In real life, the resource compiler will accept an RCDATA statement that does reference an external file. Also, the docs say RCDATA can have VERSION, CHARACTERISTICS, and LANGUAGE optional statements, but those don't seem to work because the keywords are mistaken for file names.

May 9 2018, 8:36 AM
amccarth accepted D46579: [llvm-rc] Handle C preprocessor output.
May 9 2018, 8:36 AM

May 8 2018

amccarth added inline comments to D46579: [llvm-rc] Handle C preprocessor output.
May 8 2018, 4:03 PM
amccarth accepted D46507: [llvm-rc] Add support for all missing dialog controls.

Thanks!

May 8 2018, 1:13 PM
amccarth added inline comments to D46507: [llvm-rc] Add support for all missing dialog controls.
May 8 2018, 11:07 AM
amccarth added inline comments to D46507: [llvm-rc] Add support for all missing dialog controls.
May 8 2018, 10:35 AM

May 7 2018

amccarth added inline comments to D46427: [PDB] Quote linker arguments containing spaces (mimic MSVC).
May 7 2018, 3:54 PM · lld
amccarth accepted D46511: [llvm-rc] Don't strictly require quotes around external file names.

I'm happy. Thanks!

May 7 2018, 2:21 PM
amccarth accepted D46509: [llvm-rc] Implement the BITMAP resource type.
May 7 2018, 1:21 PM
amccarth accepted D46511: [llvm-rc] Don't strictly require quotes around external file names.
May 7 2018, 11:48 AM
amccarth added inline comments to D46511: [llvm-rc] Don't strictly require quotes around external file names.
May 7 2018, 11:38 AM
amccarth added inline comments to D46509: [llvm-rc] Implement the BITMAP resource type.
May 7 2018, 11:28 AM
amccarth accepted D46508: [llvm-rc] Allow optional commas between the string table index and value.

As I recall, commas may be optionally allowed in several places besides string tables. We'll have to check that and, if necessary, address in a subsequent patch.

May 7 2018, 11:23 AM
amccarth accepted D46507: [llvm-rc] Add support for all missing dialog controls.

Looks good. AFAICT we're now only missing PUSHBOX and CONTROL. If these don't present any additional difficulty, maybe better to just add them now for the sake of completeness?

May 7 2018, 11:19 AM
amccarth accepted D46510: [llvm-rc] Exclude padding from sizes in versioninfo resources.
May 7 2018, 11:16 AM
amccarth added inline comments to D46511: [llvm-rc] Don't strictly require quotes around external file names.
May 7 2018, 11:07 AM

May 2 2018

amccarth accepted D46358: [llvm-cvtres] Allow parameters preceded by '-' in addition to '/'.

I'm mildly concerned about the tests that give filenames with forward slashes. That doesn't work with a lot of Windows command lines, but apparently the tests here were already doing that.

May 2 2018, 2:00 PM
amccarth accepted D46239: [llvm-rc] Default to writing the output next to the input, if no output is specified.

LGTM

May 2 2018, 1:54 PM
amccarth accepted D46238: [llvm-rc] Add rudimentary support for codepages.

LGTM.

May 2 2018, 11:42 AM

Apr 30 2018

amccarth accepted D46292: Use the UUID from the minidump's CodeView Record for placeholder modules.

The big picture here is fine.

Apr 30 2018, 3:52 PM · Restricted Project
amccarth added a comment to D46238: [llvm-rc] Add rudimentary support for codepages.

Looks good to me now, but I'm not an official reviewer.

Apr 30 2018, 1:50 PM
amccarth added inline comments to D46238: [llvm-rc] Add rudimentary support for codepages.
Apr 30 2018, 11:53 AM
amccarth added inline comments to D46238: [llvm-rc] Add rudimentary support for codepages.
Apr 30 2018, 10:10 AM