amccarth (Adrian McCarthy)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 26 2015, 9:26 AM (191 w, 1 d)

Recent Activity

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

Apr 27 2018

amccarth added a comment to D46164: Support: assume `std::is_final` with MSVC.

This looks fine to me. I just dropped in to add some context on MSVC and __cplusplus: https://blogs.msdn.microsoft.com/vcblog/2018/04/09/msvc-now-correctly-reports-__cplusplus/

Apr 27 2018, 7:50 AM

Apr 19 2018

amccarth committed rL330354: Fix narrowing warning by appending `f` to literal constant..
Fix narrowing warning by appending `f` to literal constant.
Apr 19 2018, 11:35 AM

Apr 18 2018

amccarth accepted D45700: Improve LLDB's handling of non-local minidumps.

I actually preferred the factory solution.

Apr 18 2018, 1:51 PM · Restricted Project

Apr 17 2018

amccarth accepted D45700: Improve LLDB's handling of non-local minidumps.

LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.

Apr 17 2018, 4:49 PM · Restricted Project

Apr 16 2018

amccarth added a comment to D45700: Improve LLDB's handling of non-local minidumps.

Nice!

Apr 16 2018, 2:48 PM · Restricted Project
amccarth committed rL330135: Remove faulty assertion in llvm-pdbutil.
Remove faulty assertion in llvm-pdbutil
Apr 16 2018, 10:05 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Apr 16 2018, 10:05 AM

Apr 13 2018

amccarth added a comment to D45645: Remove faulty assertion in llvm-pdbutil.

I meant to mention that the instance of the empty class is reported as 8 bytes of padding (in a 64-bit build), in case that's relevant to anybody's understanding of the issue.

Apr 13 2018, 4:44 PM
amccarth created D45645: Remove faulty assertion in llvm-pdbutil.
Apr 13 2018, 4:41 PM

Apr 9 2018

amccarth accepted D45345: [annotated_builder] try harder to clean build directories.

Yes, by all means. My intent was not to block this but to point out that it might still suffer from an already existent problem.

Apr 9 2018, 4:07 PM
amccarth added a comment to D45345: [annotated_builder] try harder to clean build directories.

This looks like an improvement, but it still might not work on Windows, because there you're racing the file system--deleting a file on Windows is not an atomic, instantaneous operation. Tune in to https://www.youtube.com/watch?v=uhRWMGBjlO8 at 7:30 for details.

Apr 9 2018, 2:29 PM

Mar 22 2018

amccarth added inline comments to D44810: [PDB] Make LLD PDBs look a little more like Microsoft PDBs.
Mar 22 2018, 4:00 PM
amccarth added a comment to D44771: [lit] Test /dev/null support on Windows..
In D44771#1046003, @rnk wrote:

I have a feeling if you just say open("NUL", mode) you will create a file called "NUL" in the cwd. It's not something you can feed to generic filesystem APIs.

Mar 22 2018, 1:57 PM

Feb 26 2018

amccarth abandoned D43784: Un-XFAIL TestCallStdStringFunction.test for Windows.

Abandoning. This isn't really working on Windows. The breakpoint fails to set for reasons I don't understand yet.

Feb 26 2018, 4:44 PM
amccarth added a comment to D43784: Un-XFAIL TestCallStdStringFunction.test for Windows.

Nice catch. I'll take a closer look to see what exactly is happening on Windows.

Feb 26 2018, 2:34 PM
amccarth created D43784: Un-XFAIL TestCallStdStringFunction.test for Windows.
Feb 26 2018, 1:49 PM
amccarth committed rL326130: Partial fix for TestConflictingSymbol.py on Windows.
Partial fix for TestConflictingSymbol.py on Windows
Feb 26 2018, 1:27 PM
amccarth closed D43688: Partial fix for TestConflictingSymbol.py on Windows.
Feb 26 2018, 1:27 PM
amccarth committed rL326095: Fix tabs/spaces indentation problem in TestUnicodeSymbols.py.
Fix tabs/spaces indentation problem in TestUnicodeSymbols.py
Feb 26 2018, 7:57 AM
amccarth closed D43705: Fix tabs/spaces in TestUnicodeSymbols.py.
Feb 26 2018, 7:57 AM

Feb 23 2018

amccarth created D43705: Fix tabs/spaces in TestUnicodeSymbols.py.
Feb 23 2018, 5:20 PM
amccarth added inline comments to D43596: Replace HashStringUsingDJB with llvm::djbHash.
Feb 23 2018, 5:00 PM
amccarth added a comment to D43662: [Utility] Simplify and generalize the CleanUp helper, NFC.

Nit: As a single word, "cleanup" is a noun (or an adjective). As two words, "clean up" is a verb. Given that, I'd expect to see classes and objects with names like Cleanup or cleanup and functions to contain CleanUp. When I see an identifier like CleanUpTest, I expect it's a function that cleans up after a test, not a test of a class called CleanUp.

Feb 23 2018, 2:34 PM
amccarth added a comment to D43688: Partial fix for TestConflictingSymbol.py on Windows.

On non-windows systems we're expected to pick up the name from the symbol table. Is there such a thing on windows as well?

Given that constant is declared with visibility("hidden"), why would it end up in a symbol table? Windows DLLs have tables for items explicitly exported, but that's contrary to asking the symbol to be hidden.

visibility((hidden)) means it won't end up in the *exported* symbol table (.dynsym on linux). However, the compiler will still put it into the local symbol table (.symtab, which is not used by the linker or at runtime, but the debugger can use it to get more insight).

Feb 23 2018, 2:18 PM
amccarth added a comment to D43688: Partial fix for TestConflictingSymbol.py on Windows.

This test deliberately compiles without -g, so the variable should not be in the debug info.

Feb 23 2018, 11:42 AM
amccarth created D43688: Partial fix for TestConflictingSymbol.py on Windows.
Feb 23 2018, 11:13 AM
amccarth added a comment to D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows..

That's weird, because lots of lldb tests compile and link test binaries on Windows with -fuse-ld=lld (without the .exe). What makes you say the .exe is necessary?

Maybe he is using clang (not clang-cl) and want to use ld.lld.exe instead of lld-link?

Feb 23 2018, 7:52 AM · Restricted Project

Feb 22 2018

amccarth committed rL325838: Fix llvm-pdbutil to handle new built-in types.
Fix llvm-pdbutil to handle new built-in types
Feb 22 2018, 3:21 PM
amccarth closed D43646: Fix llvm-pdbutil to handle new built-in types.
Feb 22 2018, 3:21 PM
amccarth created D43646: Fix llvm-pdbutil to handle new built-in types.
Feb 22 2018, 3:06 PM
amccarth committed rL325836: Fix TestMoveNearest on Windows.
Fix TestMoveNearest on Windows
Feb 22 2018, 2:50 PM
amccarth committed rL325835: Fix TestSBData.py on Windows.
Fix TestSBData.py on Windows
Feb 22 2018, 2:50 PM
amccarth closed D43600: Fix TestMoveNearest on Windows.
Feb 22 2018, 2:50 PM
amccarth closed D43532: Fix TestSBData.py on Windows (which uses Python 3).
Feb 22 2018, 2:49 PM
amccarth added a comment to D43600: Fix TestMoveNearest on Windows.

call_foo1() is defined in foo.cpp, which is built into a DLL, and thus needs the __declspec(dllimport/dllexport) from the LLDB_TEST_API. foo.h defines the API for the DLL.

Feb 22 2018, 2:41 PM
amccarth added a comment to D43600: Fix TestMoveNearest on Windows.

Do we not need call_foo2 anymore?

Feb 22 2018, 2:13 PM
amccarth added a comment to D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows..

Right now, we have to add an .exe suffix when using this switch, like -fuse-ld=lld.exe.

Feb 22 2018, 10:35 AM · Restricted Project
amccarth added a comment to D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows..

This must be new-ish code, since I've routinely used -fuse-ld=lld (without .exe) on Windows.

Feb 22 2018, 8:04 AM · Restricted Project

Feb 21 2018

amccarth abandoned D43599: FFix TestSBData.py on Windows (which uses Python 3).

Please ignore. I'm still trying to figure out arc.

Feb 21 2018, 4:21 PM
amccarth created D43600: Fix TestMoveNearest on Windows.
Feb 21 2018, 4:20 PM
amccarth created D43599: FFix TestSBData.py on Windows (which uses Python 3).
Feb 21 2018, 4:19 PM
amccarth committed rL325704: Fix TestBreakpointInGlobalConstructor for Windows.
Fix TestBreakpointInGlobalConstructor for Windows
Feb 21 2018, 10:10 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Feb 21 2018, 10:10 AM
amccarth updated the diff for D43419: Fix TestBreakpointInGlobalConstructor for Windows.

Per Pavel's suggestion, change special value to mean don't check the number of locations found.

Feb 21 2018, 9:05 AM
amccarth added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

By unconditional, do you mean allowing any value for out_num_locations in these cases? I'm happy to do that, but I'm not sure if I've understood you correctly.

Feb 21 2018, 8:03 AM

Feb 20 2018

amccarth created D43532: Fix TestSBData.py on Windows (which uses Python 3).
Feb 20 2018, 3:18 PM
amccarth added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

At some point we should introduce "platformCapacities" so that you could do:

if not test.getPlatformCapacities("preload-dylibs"):

so we are testing specific features not the whole platform.

Feb 20 2018, 12:45 PM
amccarth updated the diff for D43419: Fix TestBreakpointInGlobalConstructor for Windows.

Wrapped the modified docstring.

Feb 20 2018, 12:44 PM
amccarth updated the diff for D43419: Fix TestBreakpointInGlobalConstructor for Windows.

Added new special value to accommodate the fact that Windows may postpone resolving the breakpoints for DLLs that aren't yet loaded.

Feb 20 2018, 12:01 PM
amccarth added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

I have reservations about making the debugger try to pre-locate the modules and their PDBs before those modules are actually loaded. For a few reasons.

Feb 20 2018, 9:37 AM

Feb 16 2018

amccarth added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

OK, so my fix is too simplistic. I'll have to (1) make LLDB on Windows also attempt to pre-load the DLLs, (2) make the test use platform-specific expectations, or (3) use a looser expectation. I'll give it some thought over the weekend.

Feb 16 2018, 4:27 PM