Page MenuHomePhabricator

amccarth (Adrian McCarthy)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 26 2015, 9:26 AM (311 w, 5 d)

Recent Activity

Wed, Jan 13

amccarth accepted D92419: [Support] On Windows, take the affinity mask into account.

Thanks for the changes!

Wed, Jan 13, 3:59 PM · Restricted Project

Mon, Jan 11

amccarth accepted D91176: [20/N] [libcxx] Implement parsing of root_name for paths on windows.
Mon, Jan 11, 2:00 PM · Restricted Project

Fri, Jan 8

amccarth added a comment to D91176: [20/N] [libcxx] Implement parsing of root_name for paths on windows.

This looks good to me, in that I believe it will correctly parse typical native Windows path names. Only tests will tell, but I understand that there are many patches necessary to enable the tests.

Fri, Jan 8, 5:10 PM · Restricted Project
amccarth accepted D91853: [compiler-rt] [sanitizer] Silence -Wframe-larger-than= for a few windows functions with large stack buffers.

LGTM, but please take the suggested edit to update a documentation URL.

Fri, Jan 8, 12:11 PM · Restricted Project
amccarth accepted D91178: [22/N] [libcxx] Implement append and operator/ properly for windows.

LGTM.

Fri, Jan 8, 11:44 AM · Restricted Project
amccarth accepted D91171: [16/N] [libcxx] Implement the permissions function for windows.

LGTM.

Fri, Jan 8, 11:43 AM · Restricted Project

Tue, Jan 5

amccarth added a comment to D91852: [compiler-rt] [windows] Add UNUSED attributes on variables/functions only used for 64 bit targets.

Ping @amccarth, do you want to follow up on the discussion before I land this?

Tue, Jan 5, 1:56 PM · Restricted Project

Dec 17 2020

amccarth accepted D93456: [libcxx] Avoid overflows in the windows __libcpp_steady_clock_now().

This looks correct and straightforward, so LGTM.

Dec 17 2020, 9:08 AM · Restricted Project

Dec 11 2020

amccarth accepted D91172: [17/N] [libcxx] Implement the read_symlink function for windows.

LGTM.

Dec 11 2020, 2:36 PM · Restricted Project
amccarth accepted D91170: [15/N] [libcxx] Implement the canonical function for windows.

LGTM.

Dec 11 2020, 2:28 PM · Restricted Project
amccarth accepted D91169: [14/N] [libcxx] Implement the current_path function for windows.

Your call on using the CRT. I was under the mistaken impression that libcxx would implement both the standard C++ library as well as the CRT.

Dec 11 2020, 2:23 PM · Restricted Project
amccarth accepted D91179: [23/N] [libcxx] Have lexically_normal return the path with preferred separators.
Dec 11 2020, 11:44 AM · Restricted Project
amccarth requested changes to D91178: [22/N] [libcxx] Implement append and operator/ properly for windows.
Dec 11 2020, 11:41 AM · Restricted Project

Dec 10 2020

amccarth added inline comments to D92419: [Support] On Windows, take the affinity mask into account.
Dec 10 2020, 5:19 PM · Restricted Project
amccarth accepted D91177: [21/N] [libcxx] Implement is_absolute properly for windows.

LGTM iff this function never has to deal with paths prefixed with \\?\, as in \\?\C:\Windows or \\?\UNC\server\foo.

Dec 10 2020, 4:55 PM · Restricted Project

Dec 8 2020

amccarth accepted D91175: [19/N] [libcxx] Implement temp_directory_path using GetTempPath on windows.

LGTM.

Dec 8 2020, 1:56 PM · Restricted Project

Dec 7 2020

amccarth requested changes to D91175: [19/N] [libcxx] Implement temp_directory_path using GetTempPath on windows.
Dec 7 2020, 2:58 PM · Restricted Project
amccarth added a comment to D91173: [18/N] [libcxx] Use the posix code for directory_entry::__do_refresh.

Oh, are there tests that need to be enabled on Windows?

Dec 7 2020, 1:16 PM · Restricted Project
amccarth accepted D91173: [18/N] [libcxx] Use the posix code for directory_entry::__do_refresh.

LGTM.

Dec 7 2020, 1:16 PM · Restricted Project
amccarth accepted D92372: replacing `rm -rf` with RemoveDirectory step in ClangBuilders.

I'm glad @gkistanova asked for this simplification. The more cautious approach added a lot of probably unnecessary noise.

Dec 7 2020, 9:33 AM

Dec 4 2020

amccarth added a comment to D91172: [17/N] [libcxx] Implement the read_symlink function for windows.

This looks like it'll work just fine, but I've added some inline questions for readability and robustness.

Dec 4 2020, 3:54 PM · Restricted Project
amccarth requested changes to D91171: [16/N] [libcxx] Implement the permissions function for windows.
Dec 4 2020, 11:18 AM · Restricted Project

Dec 3 2020

amccarth requested changes to D91170: [15/N] [libcxx] Implement the canonical function for windows.
Dec 3 2020, 2:56 PM · Restricted Project
amccarth accepted D91145: [12/N] [libcxx] Sanitize paths before creating symlinks on windows.

LGTM

Dec 3 2020, 2:25 PM · Restricted Project

Dec 2 2020

amccarth added inline comments to D91169: [14/N] [libcxx] Implement the current_path function for windows.
Dec 2 2020, 4:53 PM · Restricted Project
amccarth accepted D91168: [13/N] [libcxx] Implement the space function for windows.

LGTM.

Dec 2 2020, 3:45 PM · Restricted Project
amccarth accepted D91143: [11/N] [libcxx] Hook up a number of operation functions to their windows counterparts.

LGTM, now.

Dec 2 2020, 3:15 PM · Restricted Project
amccarth resigned from D91139: [7/N] [libcxx] Don't use __int128 for msvc targets.

I'm out of my depth on 128-bit arithmetic and ABIs.

Dec 2 2020, 3:10 PM · Restricted Project

Dec 1 2020

amccarth added inline comments to D91138: [6/N] [libcxx] Handle backslash as path separator on windows.
Dec 1 2020, 2:56 PM · Restricted Project
amccarth added inline comments to D91135: [3/N] [libcxx] Make filesystem::path::value_type wchar_t on windows.
Dec 1 2020, 1:55 PM · Restricted Project
amccarth added a comment to D91133: [2/N] [libcxx] [test] Add a test for conversions between wchar_t, utf8, char16_t, char32_t and windows native narrow code pages.

These test look pretty thorough. I think these will be useful in finding portability problems.

Dec 1 2020, 1:41 PM · Restricted Project
amccarth added a comment to D92372: replacing `rm -rf` with RemoveDirectory step in ClangBuilders.

I think this patch is a good idea, but I'm having second thoughts about whether this will solve the problem.

Dec 1 2020, 10:29 AM
amccarth accepted D92372: replacing `rm -rf` with RemoveDirectory step in ClangBuilders.

LGTM. (I must have slipped on the Action drop-down.)

Dec 1 2020, 10:19 AM
amccarth requested changes to D92372: replacing `rm -rf` with RemoveDirectory step in ClangBuilders.

I look forward to just switching everything to RemoveDirectory.

Dec 1 2020, 10:18 AM

Nov 30 2020

amccarth added a comment to D91853: [compiler-rt] [sanitizer] Silence -Wframe-larger-than= for a few windows functions with large stack buffers.

I prefer the relatively localized disabling of the warning with the #pragmas as you've done as opposed to building the sanitizer builds with different settings.

Nov 30 2020, 4:56 PM · Restricted Project
amccarth accepted D91852: [compiler-rt] [windows] Add UNUSED attributes on variables/functions only used for 64 bit targets.
Nov 30 2020, 4:41 PM · Restricted Project

Nov 20 2020

amccarth accepted D91814: [llvm-symbolizer] Switch to using native symbolizer by default on Windows.

LGTM

Nov 20 2020, 1:22 PM · Restricted Project
amccarth accepted D91142: [10/N] [libcxx] Implement _FilesystemClock::now() and __last_write_time for windows.

LGTM. I'll leave it to your discretion on the new_time range check.

Nov 20 2020, 11:18 AM · Restricted Project

Nov 19 2020

amccarth added a comment to D91142: [10/N] [libcxx] Implement _FilesystemClock::now() and __last_write_time for windows.

This looks good. I'm just confused by one point and have a small suggested edit.

Nov 19 2020, 3:32 PM · Restricted Project
amccarth added a comment to D91814: [llvm-symbolizer] Switch to using native symbolizer by default on Windows.

I added two suggested edits for your consideration.

Nov 19 2020, 2:49 PM · Restricted Project

Nov 18 2020

amccarth added a comment to D91141: [9/N] [libcxx] Implement the stat function family on top of native windows APIs.

LGTM as long as the run-time calculation of the default value for set_errno is legit.

Nov 18 2020, 9:59 AM · Restricted Project
amccarth accepted D91140: [8/N] [libcxx] Fix the preexisting directory_iterator code for windows.

I'm not a libcxx owner, so I'm not sure what other hoops you should jump through for them, but this looks good to me. Thanks for the improvements!

Nov 18 2020, 9:35 AM · Restricted Project

Nov 17 2020

amccarth accepted D91137: [5/N] [libcxx] Convert paths to/from the right narrow code page for narrow strings on windows.

Excellent. Somehow I missed that AreFileApisANSI was a Win32 function. I must have mistyped it in my searches the other day. This looks good.

Nov 17 2020, 1:28 PM · Restricted Project

Nov 16 2020

amccarth added a comment to D91137: [5/N] [libcxx] Convert paths to/from the right narrow code page for narrow strings on windows.

The Windows details look correct, though I have a couple questions on the code page conversions.

Nov 16 2020, 2:38 PM · Restricted Project

Nov 12 2020

amccarth added a comment to D91143: [11/N] [libcxx] Hook up a number of operation functions to their windows counterparts.

I have a couple questions to consider inline. If those have already been thought through, then LTGM.

Nov 12 2020, 11:26 AM · Restricted Project
amccarth requested changes to D91140: [8/N] [libcxx] Fix the preexisting directory_iterator code for windows.

This is pretty close.

Nov 12 2020, 10:15 AM · Restricted Project

Oct 23 2020

amccarth added a comment to D89812: [lldb][PDB] Add ObjectFile PDB plugin.

This looks pretty good, both the patch and Pavel's insights. I don't see much to comment on that Pavel didn't already catch.

Oct 23 2020, 5:34 PM · Restricted Project

Oct 19 2020

amccarth accepted D89702: [clang] [msvc] Automatically link against oldnames just as linking against libcmt.

It seems weird that we're implicitly adding defaultlib options without checking if the user specified nodefaultlib. But given that's already the case, I don't see a big problem with also adding oldnames.lib.

Oct 19 2020, 1:30 PM · Restricted Project

Oct 15 2020

amccarth updated subscribers of D84380: [lit] Support running tests on Windows without GnuWin32..

I have no reason to suspect those patches in particular. I did a huge git
bisect, which turned up nothing. Older versions that used to work no
longer worked by the time the bisect worked that far back. That makes me
think it might be incomplete dependency checking: Something goes bad, and,
instead of rebuilding it, the bad result is re-used in each subsequent
build.

Oct 15 2020, 8:42 AM · Restricted Project

Oct 14 2020

amccarth added a comment to D84380: [lit] Support running tests on Windows without GnuWin32..

Also, worth nothing that C:\Program Files\Git\usr\bin is not enough to run the lldb tests, which need make and maybe other things.
I'm not sure if that was part of your original goal, but since clang changes can affect lldb behavior, that means developers would still need some additionnal tools along Git for Windows.

One more note: even after installing make with choco install make and adding it to %PATH%, many lldb tests still fail because we call sh F:\llvm-project\build\foo and the backslashes are escaped by sh. We end up with F:lvm-projectuildoo something like this. We could maybe insert cygpath before the calls to sh to fix that in the lldb scripts.

Oct 14 2020, 11:38 AM · Restricted Project

Oct 13 2020

amccarth updated subscribers of D88666: DirectoryWatcher: add an implementation for Windows.

If I had to guess, my money would be on a deadlock. To unblock, I'd
propose reverting this patch until we can figure it out.

Oct 13 2020, 9:22 AM · Restricted Project

Oct 12 2020

amccarth committed rGf21fcccef719: [LLDB] Fix 37 tests on Windows (authored by amccarth).
[LLDB] Fix 37 tests on Windows
Oct 12 2020, 11:15 AM
amccarth closed D89256: [LLDB] Fix 37 tests on Windows.
Oct 12 2020, 11:15 AM · Restricted Project
amccarth requested review of D89256: [LLDB] Fix 37 tests on Windows.
Oct 12 2020, 11:03 AM · Restricted Project

Oct 9 2020

amccarth accepted D88666: DirectoryWatcher: add an implementation for Windows.

LGTM. Thanks for extending this functionality to Windows!

Oct 9 2020, 8:57 AM · Restricted Project

Oct 8 2020

amccarth added a comment to D88988: [llvm-symbolizer] Add inline stack traces for Windows..

This looks pretty good.

Oct 8 2020, 4:20 PM · Restricted Project, Restricted Project

Oct 7 2020

amccarth added a comment to D88967: [lldb] Add a cmake warning about the python/swig incompatibility.

If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious.

Is that problem only theoretical (like, "you shouldn't be doing that") or does it have some practical consequences (crashes, incorrect operation, etc.)?

Oct 7 2020, 2:07 PM · Restricted Project
amccarth added a comment to D88967: [lldb] Add a cmake warning about the python/swig incompatibility.

If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious.

Oct 7 2020, 11:32 AM · Restricted Project
amccarth added a comment to D88975: [LLDB] On Windows, fix tests.

It's interesting that I haven't encountered some of these errors. There are five _other_ lldb tests that do fail for me. I have a fix in the works for some of those.

Oct 7 2020, 11:25 AM · Restricted Project

Oct 6 2020

amccarth requested changes to D88666: DirectoryWatcher: add an implementation for Windows.

Some of this is nitpicky/opinionated, but the race condition is real. We need a reliable way to signal the watcher thread when it's time to exit. Options I see are:

Oct 6 2020, 2:10 PM · Restricted Project
amccarth added a comment to D84380: [lit] Support running tests on Windows without GnuWin32..

It seems confusing to me to automagically use tools that are not on PATH (or passed in explicitly as cmake flags or similar).

Oct 6 2020, 12:59 PM · Restricted Project
amccarth accepted D88906: [lldb/docs] Clarify python/swig version incompatibility.

Yes, I also (independently) discovered this problem. I probably have notes somewhere with sources for details about the incompatibility. I believe I also brought it up on the lldb-dev list.

Oct 6 2020, 10:10 AM · Restricted Project

Oct 5 2020

amccarth added a comment to D88850: [lit, windows] Fix the search for git tools on Windows to check the path first.

I don't know whether Python and our config tools use the terms consistent, but Win32 is the name of the Windows API used on both 32- and 64-bit processors, Win16 was the API used on 16-bit machines, and Win64 is a made-up term that confuses the issues.

Oct 5 2020, 3:07 PM · Restricted Project

Oct 2 2020

amccarth added a comment to D88666: DirectoryWatcher: add an implementation for Windows.

I'm sorry. I haven't had to time to review the entire change yet, but I thought I'd share some early feedback now and take more of a look on Monday.

Oct 2 2020, 5:28 PM · Restricted Project
amccarth added a comment to D84380: [lit] Support running tests on Windows without GnuWin32..
In D84380#2307030, @rnk wrote:

Figured it out, Python 2 vs 3 bug:
rGd12ae042e17b27ebc8d2b5ae3d8dd5f88384d093

Oct 2 2020, 9:05 AM · Restricted Project

Sep 29 2020

amccarth added a comment to D88401: [lib/Support/CommandLine][windows] - Handle "\ " as a space token..
Sep 29 2020, 4:40 PM · Restricted Project

Sep 25 2020

amccarth added a comment to D87732: [Support] Provide sys::path::guess_style.
In D87732#2296076, @rnk wrote:

Separately, I would like LLVM to move in the direction of standardizing on forward slashes in internal representations and data structures.

Sep 25 2020, 5:25 PM · Restricted Project
amccarth added a comment to D87732: [Support] Provide sys::path::guess_style.

A big portability problem with the virtual file system (VFS) is that we now have paths that can legitimately be in a hybrid Windows/Posix style. Be aware that some of the "guess-the-style" instances were modified to account for this.

Sep 25 2020, 8:45 AM · Restricted Project

Sep 21 2020

amccarth accepted D70378: [LLD][COFF] Cover usage of LLD as a library.

LGTM.

Sep 21 2020, 8:37 AM · Restricted Project, lld, Restricted Project

Sep 15 2020

amccarth accepted D87570: [llvm-rc] Allow omitting components from VERSIONINFO version numbers.

I never realized rc.exe would accept having too few values for a FILEVERSION or PRODUCTVERSION. The documentation implies that you must provide four components--it is the _fixed_ portion of the VERSIONINFO after all. Not that it matters; this looks harmless.

Sep 15 2020, 8:48 AM · Restricted Project

Sep 2 2020

amccarth added a comment to D85993: [lldb] Set the access property on member function decls.

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

Sep 2 2020, 3:46 PM

Aug 31 2020

amccarth accepted D86701: [LLD] [COFF] Error out if creating a DLL with too many exported symbols.

I like it.

Aug 31 2020, 8:57 AM · Restricted Project

Aug 28 2020

amccarth added a comment to D86762: [ELF] Add documentation for --warn-backrefs: a GNU ld compatibility checking tool (and lesser of layering detection).

Is there a more specific term than "traditional linker"?

Aug 28 2020, 9:02 AM · Restricted Project

Aug 27 2020

amccarth added a comment to D86701: [LLD] [COFF] Error out if creating a DLL with too many exported symbols.

The change looks fine.

Aug 27 2020, 9:35 AM · Restricted Project

Aug 26 2020

amccarth accepted D86659: [LLD] [COFF] Check the aux section definition size for IMAGE_COMDAT_SELECT_SAME_SIZE.

Oops, I forgot to "Accept Revision."

Aug 26 2020, 2:56 PM · Restricted Project
amccarth added a comment to D86659: [LLD] [COFF] Check the aux section definition size for IMAGE_COMDAT_SELECT_SAME_SIZE.

Not my area of expertise, but this looks good to me. Before submitting, please make sure you update the commit description since you didn't end up adding any fields to SectionChunk.

Aug 26 2020, 2:55 PM · Restricted Project
amccarth accepted D86654: [LLD] [MinGW] Enable dynamicbase by default.

LGTM.

Aug 26 2020, 2:34 PM · Restricted Project
amccarth accepted D86564: [Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle().

LGTM

Aug 26 2020, 9:47 AM · Restricted Project
amccarth requested changes to D86564: [Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle().

I know it's not part of this patch, but Phabricator is showing a clang-tidy lint error on the inclusion of <io.h>. While Windows does have one of these, it's only a compatibility hack, and it doesn't seem to be needed. Can you yank that #include <io.h> line before submitting?

Aug 26 2020, 8:57 AM · Restricted Project

Aug 14 2020

amccarth requested review of D85993: [lldb] Set the access property on member function decls.
Aug 14 2020, 1:24 PM

Aug 13 2020

amccarth closed D84815: [LLDB] Improve PDB discovery.

Landed on Tuesday. I must have messed up the Differential Revision: line.

Aug 13 2020, 4:17 PM

Aug 12 2020

amccarth committed rG7ddfb956e1a5: [lldb] Fix unit test parsing to handle CR+LF as well as LF (authored by amccarth).
[lldb] Fix unit test parsing to handle CR+LF as well as LF
Aug 12 2020, 1:56 PM

Aug 11 2020

amccarth committed rG479f5bfdb02b: [LLDB] Improve PDB discovery (authored by amccarth).
[LLDB] Improve PDB discovery
Aug 11 2020, 1:45 PM
amccarth added a comment to D85745: emit int32 for each version part.

If you make the second loop consistent, I'll approve, despite my concerns about the related problems.

Aug 11 2020, 11:02 AM · Restricted Project

Aug 10 2020

amccarth added a comment to D85669: Fix "last accessed time" test failing on Windows.

I'm OK with this.

In an ideal world, it would be nice to ensure stripping preserves other the Windows creation timestamp while updating the last modification timestamp.

Thanks Adrian. The "last modification time" is preserved with llvm-strip -p, see llvm\test\tools\llvm-objcopy\ELF\strip-preserve-mtime.test which works fine (if that's what you meant).

Aug 10 2020, 1:56 PM · Restricted Project
amccarth accepted D85669: Fix "last accessed time" test failing on Windows.
Aug 10 2020, 11:42 AM · Restricted Project
amccarth added a comment to D85669: Fix "last accessed time" test failing on Windows.

I'm OK with this.

Aug 10 2020, 11:42 AM · Restricted Project
amccarth accepted D85633: [lldb][NFC] Remove unused custom reimplementation of realpath for Windows.

Nice.

Aug 10 2020, 9:26 AM · Restricted Project

Aug 7 2020

amccarth added a comment to D85480: [NFC] Use value initializer for OVERLAPPED.

OK, I see. I was looking at the wrong field.

Aug 7 2020, 5:02 PM · Restricted Project
amccarth added a comment to D85480: [NFC] Use value initializer for OVERLAPPED.

Huh, I thought aggregate initialization was effectively zero-initialization for the remaining fields, so I'm surprised this changes anything. I would have expected the original line, which is idiomatic in Win32 code, to have zero-initialized all of the fields.

Aug 7 2020, 4:48 PM · Restricted Project
amccarth updated the diff for D84815: [LLDB] Improve PDB discovery.

Added test to check locating the PDB either in the original build directory or adjacent to the executable.

Aug 7 2020, 1:36 PM

Aug 4 2020

amccarth accepted D85183: [llvm-rc] Allow string table values split into multiple string literals.

OK, makes sense. I was hoping the processing could happen as you build up the concatenated string rather than at the end. But now I see the design doesn't really allow for that.

Aug 4 2020, 1:47 PM · Restricted Project
amccarth added a comment to D85183: [llvm-rc] Allow string table values split into multiple string literals.

I'll defer to @thakis, but I have a couple suggestions inline.

Aug 4 2020, 9:59 AM · Restricted Project

Aug 3 2020

amccarth added a comment to D84423: [lit] Allow lit.which() to find executables which already have an extension.

Well then, since Reid is out, this is good to go! :-)

Aug 3 2020, 10:26 AM · Restricted Project
amccarth accepted D84380: [lit] Support running tests on Windows without GnuWin32..

I have a couple responses, but nothing worth blocking this path for. LGTM, and thanks for doing this!

Aug 3 2020, 10:23 AM · Restricted Project

Jul 30 2020

amccarth added a comment to D84815: [LLDB] Improve PDB discovery.

Thanks. Working on a test.

Jul 30 2020, 8:06 AM

Jul 29 2020

amccarth accepted D84757: [compiler-rt] [profile] fix profile generate for mingw x86_64.

LGTM.

Jul 29 2020, 8:50 AM · Restricted Project

Jul 28 2020

amccarth updated the diff for D84815: [LLDB] Improve PDB discovery.

Fixed typos in comments.

Jul 28 2020, 6:32 PM
amccarth requested review of D84815: [LLDB] Improve PDB discovery.
Jul 28 2020, 6:14 PM
amccarth accepted D84701: [LLD] [MinGW] Implement the --no-seh flag.

LGTM!

Jul 28 2020, 8:22 AM · Restricted Project