Page MenuHomePhabricator

amccarth (Adrian McCarthy)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 26 2015, 9:26 AM (289 w, 2 d)

Recent Activity

Yesterday

amccarth committed rG479f5bfdb02b: [LLDB] Improve PDB discovery (authored by amccarth).
[LLDB] Improve PDB discovery
Tue, Aug 11, 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.

Tue, Aug 11, 11:02 AM · Restricted Project

Mon, Aug 10

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

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

I'm OK with this.

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

Nice.

Mon, Aug 10, 9:26 AM · Restricted Project

Fri, Aug 7

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

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

Fri, Aug 7, 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.

Fri, Aug 7, 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.

Fri, Aug 7, 1:36 PM

Tue, Aug 4

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.

Tue, Aug 4, 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.

Tue, Aug 4, 9:59 AM · Restricted Project

Mon, Aug 3

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! :-)

Mon, Aug 3, 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!

Mon, Aug 3, 10:23 AM · Restricted Project

Thu, Jul 30

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

Thanks. Working on a test.

Thu, Jul 30, 8:06 AM

Wed, Jul 29

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

LGTM.

Wed, Jul 29, 8:50 AM · Restricted Project

Tue, Jul 28

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

Fixed typos in comments.

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

LGTM!

Tue, Jul 28, 8:22 AM · Restricted Project

Mon, Jul 27

amccarth added a comment to D84701: [LLD] [MinGW] Implement the --no-seh flag.

This is probably fine, be I have an inline comment with some questions.

Mon, Jul 27, 3:31 PM · Restricted Project
amccarth accepted D78896: [Support] Add file lock/unlock functions.

As I noted on April 30, I'm OK with the Windows portions of this. I didn't explicitly "Accept" because I didn't want to pre-empt the concerns of the other reviewers.

Mon, Jul 27, 10:33 AM · Restricted Project

Thu, Jul 23

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

This looks fine to me. Is there a lit expert/owner who should also review?

Thu, Jul 23, 11:22 AM · Restricted Project
amccarth requested changes to D84380: [lit] Support running tests on Windows without GnuWin32..

I'm okay with the idea. I haven't used GnuWin32 in long while since I already have Git's usr/bin in my PATH. See detailed comments inline.

Thu, Jul 23, 10:52 AM · Restricted Project

Wed, Jul 22

amccarth accepted D84286: [Symbolize][PDB] Switch llvm-symbolizer to use PDB_ReaderType::Native..

Fingers crossed...

Wed, Jul 22, 8:29 AM · Restricted Project

Tue, Jul 21

amccarth accepted D84208: [PDB][NativeSession] Clean up some things in NativeSession..

The naming improvements really help. And it's nice to see the symbol tag set correctly (rather than generically only to be overridden). Nice work.

Tue, Jul 21, 4:00 PM · Restricted Project
amccarth added a comment to rGd4020ef7c474: [Windows] Fix limit on command line size.

It seems @sepavloff has already reverted, so I'll assume they'll fix it.

Tue, Jul 21, 3:43 PM
amccarth added a comment to D83772: [Windows] Fix limit on command line size.

This commit is now failing LLDB Windows buildbot builds http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/17702. Please fix or revert.

E:\build_slave\lldb-x64-windows-ninja\llvm-project\lldb\source\Host\windows\ProcessLauncherWindows.cpp(53): error C2679: binary '=': no operator found which takes a right-hand operand of type 'llvm::ErrorOr<std::wstring>' (or there is no acceptable conversion)
Tue, Jul 21, 3:00 PM · Restricted Project, Restricted Project
amccarth added a comment to D84038: On Windows build, making the /bigobj flag global , instead of passing it per file..

Just a few days ago, I landed another single file /bigobj to get some LLDB tests running: https://reviews.llvm.org/rG14dde438d69c81ab4651157a94d32e5555e804ff

Tue, Jul 21, 2:07 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Fri, Jul 17

amccarth accepted D84070: [LLDB] [COFF] Fix handling of symbols with more than one aux symbol.

LGTM. That was a nice catch on the other review.

Fri, Jul 17, 1:47 PM · Restricted Project
amccarth committed rG14dde438d69c: With MSVC, file needs to be compiled with /BIGOBJ (authored by amccarth).
With MSVC, file needs to be compiled with /BIGOBJ
Fri, Jul 17, 9:43 AM
amccarth closed D83991: With MSVC, file needs to be compiled with /BIGOBJ.
Fri, Jul 17, 9:43 AM · Restricted Project

Thu, Jul 16

amccarth created D83991: With MSVC, file needs to be compiled with /BIGOBJ.
Thu, Jul 16, 3:25 PM · Restricted Project
amccarth committed rG72958c9ab1cd: [lldb] Eliminated unused local variable (authored by amccarth).
[lldb] Eliminated unused local variable
Thu, Jul 16, 3:02 PM
amccarth accepted D83881: [lldb/COFF] Remove strtab zeroing hack.

Thanks @mstorsjo for clarifying for me.

Thu, Jul 16, 11:46 AM · Restricted Project

Wed, Jul 15

amccarth added a comment to D83881: [lldb/COFF] Remove strtab zeroing hack.

Yes, getting rid of this hack looks like a good idea. If it was actually necessary, there should have been a test on it, and the comments should have been clearer.

Wed, Jul 15, 9:59 AM · Restricted Project
amccarth accepted D83772: [Windows] Fix limit on command line size.

Thanks! I know it was more work, but I think unifying the command line generation to work the same in both cases will avoid future surprises.

Wed, Jul 15, 8:15 AM · Restricted Project, Restricted Project

Tue, Jul 14

amccarth accepted D83720: [LLD] [MinGW] Implement the --file-alignment and --section-alignment options.

Sorry, I thought I had LGTM'ed yesterday. I guess the Submit didn't take.

Tue, Jul 14, 2:03 PM · Restricted Project
amccarth added a reviewer for D83772: [Windows] Fix limit on command line size: amccarth.
Tue, Jul 14, 1:39 PM · Restricted Project, Restricted Project
amccarth added a comment to D83772: [Windows] Fix limit on command line size.

I'm jumping in since rnk is on leave and (I believe) zturner is less focused on these issues than he used to be.

Tue, Jul 14, 1:39 PM · Restricted Project, Restricted Project

Mon, Jul 13

amccarth added a comment to D83721: [LLD] [MinGW] Ignore the --[no-]allow-shlib-undefined option.

I don't have enough background here, so I'll defer to the decision of other reviewers whether this makes sense. @ruiu ?

Mon, Jul 13, 4:38 PM · Restricted Project

Jul 1 2020

amccarth accepted D81950: [NativeSession] Add column numbers to NativeLineNumber..

LGTM

Jul 1 2020, 3:09 PM · Restricted Project

Jun 30 2020

amccarth accepted D82542: [Support][Windows] Prevent 2s delay when renaming a file that does not exist.

LGTM. Thanks!

Jun 30 2020, 9:44 AM · Restricted Project

Jun 25 2020

amccarth added a comment to D82542: [Support][Windows] Prevent 2s delay when renaming a file that does not exist.

rnk is on leave, so I'll jump in.

Jun 25 2020, 11:54 AM · Restricted Project
amccarth edited reviewers for D82542: [Support][Windows] Prevent 2s delay when renaming a file that does not exist, added: amccarth; removed: rnk.
Jun 25 2020, 11:54 AM · Restricted Project

Jun 24 2020

amccarth added a comment to D81950: [NativeSession] Add column numbers to NativeLineNumber..

Very close now. It looks like the change I requested led to a bug. Once that's corrected, this'll look great.

Jun 24 2020, 5:24 PM · Restricted Project

Jun 22 2020

amccarth accepted D82265: [llvm-rc] Implement the language id option.
Jun 22 2020, 11:17 AM · Restricted Project

Jun 16 2020

amccarth added a comment to D81950: [NativeSession] Add column numbers to NativeLineNumber..

This looks right, but I'd like to see a test.

Jun 16 2020, 5:01 PM · Restricted Project

Jun 15 2020

amccarth accepted D80962: [NativeSession] Implement findLineNumbersByAddress functions for native llvm-symbolizer support..

LGTM.

Jun 15 2020, 2:54 PM · Restricted Project
amccarth added a reviewer for D81803: [Support] PR42623: Avoid setting the delete-on-close bit for TempFile: inglorion.
Jun 15 2020, 2:20 PM · Restricted Project
amccarth updated subscribers of D81803: [Support] PR42623: Avoid setting the delete-on-close bit for TempFile.

That commit (which was reviewed here https://reviews.llvm.org/D48051) suggests this wasn't about the occasional temp file not getting deleted. It was to work around surprising behavior on Windows when a temp file is mapped into memory, which, as I recall, was the underlying cause in what appeared to be cache corruption for LTO.

Jun 15 2020, 2:20 PM · Restricted Project
amccarth accepted D81794: [clang] Don't emit warn_cxx_ms_struct when MSBitfields is enabled globally.

Thanks. This seems well-contained and the test changes look good.

Jun 15 2020, 9:45 AM · Restricted Project
amccarth added inline comments to D80962: [NativeSession] Implement findLineNumbersByAddress functions for native llvm-symbolizer support..
Jun 15 2020, 9:13 AM · Restricted Project

Jun 12 2020

amccarth added a comment to D81041: Use existing path sep style in clang::FileManager::FixupRelativePath.

After some further investigation, I have come to believe that the root cause of the issue I am seeing is on line 783 of clang/lib/Lex/HeaderSearch.cpp. A path is constructed using string concatenation (dir + '/' + file), which is obviously not robust to the various issues in path construction. A fix had been committed and reverted back in 2015.

Jun 12 2020, 3:58 PM · Restricted Project
amccarth accepted D80833: [CodeView] Add full repro to LF_BUILDINFO record.

My only real concern was about the possible reproducibility impact of introducing absolute paths into the binaries. If @thakis and @hans are satisfied this is OK, then LGTM.

Jun 12 2020, 3:58 PM · Restricted Project, Restricted Project
amccarth added a comment to D80962: [NativeSession] Implement findLineNumbersByAddress functions for native llvm-symbolizer support..

Sorry about my review latency.

Jun 12 2020, 3:58 PM · Restricted Project

Jun 3 2020

amccarth added a comment to D81058: [lldb/Interpreter] Color "error:" and "warning:" in the CommandReturnObject output..

I'm just responding to the questions @labath raised without really looking at the details of the code.

Jun 3 2020, 9:53 AM · Restricted Project
amccarth added a comment to D81041: Use existing path sep style in clang::FileManager::FixupRelativePath.

Please make sure all of the clang VFS tests still pass before submitted this. VFS paths are inherently problematic with llvm::sys::path because they can legitimately be in a hybrid of Windows and Posix styles. Guessing the style from the first separator you see can be misleading, but I don't know whether that's ever wrong in this path.

Jun 3 2020, 8:45 AM · Restricted Project
amccarth added a comment to D80833: [CodeView] Add full repro to LF_BUILDINFO record.

The change description says something about PWD

Jun 3 2020, 8:44 AM · Restricted Project, Restricted Project

Jun 2 2020

amccarth added a comment to D80833: [CodeView] Add full repro to LF_BUILDINFO record.

Does the full path to the tool end up only in the object file, or does this make it all the way to the final executable or library? Does embedding full paths affect distributed builds or build reproducibility?

Jun 2 2020, 11:32 AM · Restricted Project, Restricted Project

Jun 1 2020

amccarth accepted D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt.

Yowza. Mingw still links against MSVCRT?!

Jun 1 2020, 11:52 AM · Restricted Project

May 27 2020

amccarth committed rG2d068e534f16: Fix Windows command line bug when last token in response file is "" (authored by amccarth).
Fix Windows command line bug when last token in response file is ""
May 27 2020, 3:17 PM
amccarth closed D78346: Fix Windows command line bug when last token in response file is "".
May 27 2020, 3:17 PM · Restricted Project
amccarth added a comment to D78346: Fix Windows command line bug when last token in response file is "".

No problem. I'll land it this afternoon (Pacific time). Thanks!

May 27 2020, 1:02 PM · Restricted Project
amccarth added inline comments to D80554: [DebugInfo] Use SplitTemplateClosers (foo<bar<baz> >) in DWARF too.
May 27 2020, 1:02 PM · Restricted Project
amccarth accepted D78346: Fix Windows command line bug when last token in response file is "".

Ah, I guess the diff was based off the old head rather than the rebased one. Phabricator highlighted all that code as part of this patch.

May 27 2020, 9:11 AM · Restricted Project

May 26 2020

amccarth added inline comments to D80554: [DebugInfo] Use SplitTemplateClosers (foo<bar<baz> >) in DWARF too.
May 26 2020, 11:58 AM · Restricted Project
amccarth requested changes to D78346: Fix Windows command line bug when last token in response file is "".

Thanks for extending the tests. Those will certainly make your fix more robust against regressions.

May 26 2020, 11:58 AM · Restricted Project

May 20 2020

amccarth added a comment to D70378: [LLD][COFF] Fix missing cache cleanup in COFF::link().

I read back through some of the history, but I still don't feel like I have all the context. That said, this change looks simple and straightforward to me.

Would it be feasible and useful to create a test that links two trivial programs with a single instantiation of the lld process? It seems like it would detect regressions and catch any introduction of a new global that needs to be reset.

That is great suggestion! I will see what I can do. Maybe something along the lines of LLVM_ENABLE_ASSERTIONS + LLVM_ENABLE_EXPENSIVE_CHECKS + run all LLD tests while calling link() three times in a row?

May 20 2020, 8:43 AM · lld, Restricted Project

May 19 2020

amccarth added a comment to D70378: [LLD][COFF] Fix missing cache cleanup in COFF::link().

I read back through some of the history, but I still don't feel like I have all the context. That said, this change looks simple and straightforward to me.

May 19 2020, 3:27 PM · lld, Restricted Project
amccarth added a reviewer for D78346: Fix Windows command line bug when last token in response file is "": amccarth.

I'm happy to see this through.

May 19 2020, 1:10 PM · Restricted Project
amccarth added a comment to D78346: Fix Windows command line bug when last token in response file is "".

I think the fix looks good, but I'd like to see a bit more in the test.

May 19 2020, 9:16 AM · Restricted Project

May 15 2020

amccarth added a comment to D80006: [Clang][DebugInfo] Make DIFile filename and directory native to target architecture.

As long as the tests are working, I'm OK with this. But please hold off until others chime in. I know @thakis and others have been dealing with making cross-platform builds work better with relative paths, so they may know about more constraints on file names in debug info.

May 15 2020, 10:18 AM · debug-info, Restricted Project
amccarth added a comment to D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms..

I'll re-iterate my opinion that the casts currently required to bypass the warnings are useful (e.g., if you ever want to port the code to 64-bit). There are lots of places where the Windows API essentially required uncomfortable conversions, and we can't paper over all of them. The way to get it right in all those other places is to be very aware of the underlying types. Hiding an uncomfortable detail here and there seems like a disservice.

May 15 2020, 9:12 AM · Restricted Project

May 13 2020

amccarth committed rGa549c0d00486: Fix template class debug info for Visual Studio visualizers (authored by amccarth).
Fix template class debug info for Visual Studio visualizers
May 13 2020, 2:44 PM
amccarth closed D79274: Fix template class debug info for Visual Studio visualizers.
May 13 2020, 2:44 PM · Restricted Project
amccarth updated the diff for D79274: Fix template class debug info for Visual Studio visualizers.

Addressed feedback, specifically:

  • Distinction is now on CodeView generation rather than -fms-compatibility.
  • Tests two --std= levels plus the default one.
May 13 2020, 11:25 AM · Restricted Project
amccarth added a comment to D79274: Fix template class debug info for Visual Studio visualizers.

Made the requested changes after an in-person conversation to clear up my earlier confusion.

May 13 2020, 11:25 AM · Restricted Project

May 12 2020

amccarth accepted D79269: [NativeSession] Implement NativeSession::findSymbolByAddress..

Looks good. Thanks for the improvements.

May 12 2020, 1:59 PM · Restricted Project
amccarth added a comment to D79771: [examples] Skip building the Bye pass plugin on windows.

Windows doesn't properly support pass plugins (as a shared library can't have undefined references, which pass plugins assume, being loaded into a host process that contains provides them)

May 12 2020, 10:11 AM · Restricted Project

May 7 2020

amccarth added inline comments to D79274: Fix template class debug info for Visual Studio visualizers.
May 7 2020, 9:05 AM · Restricted Project

May 6 2020

amccarth updated the diff for D79274: Fix template class debug info for Visual Studio visualizers.

Updated an existing test. Thanks for the pointer; I had gotten myself completely confused about the organization of the test directories.

May 6 2020, 5:04 PM · Restricted Project
amccarth added a comment to D79274: Fix template class debug info for Visual Studio visualizers.

Updated test to validate the change.

May 6 2020, 5:04 PM · Restricted Project
amccarth accepted D76869: [Clang] Restore replace_path_prefix instead of startswith .

I'm less worried about the details of style (like camel casing), since I have the impression things are changing. In general, stick to prevalent style of the file when it's discernable. My issue with PrefixStyle was that the name could be confusing.

May 6 2020, 8:35 AM · Restricted Project, Restricted Project

May 5 2020

amccarth added a comment to D76869: [Clang] Restore replace_path_prefix instead of startswith .

@amccarth : Does it cover all the VFS tests you suggested that I double-check?

May 5 2020, 3:40 PM · Restricted Project, Restricted Project
amccarth added a comment to D76869: [Clang] Restore replace_path_prefix instead of startswith .

I think this is pretty close now.

May 5 2020, 3:40 PM · Restricted Project, Restricted Project
amccarth accepted D79005: [LLD][COFF] Move debug info for thread-local variables into PDB global stream.

LGTM, but I think we should hear again from rnk before landing this.

May 5 2020, 9:41 AM · Restricted Project
amccarth added inline comments to D79269: [NativeSession] Implement NativeSession::findSymbolByAddress..
May 5 2020, 9:08 AM · Restricted Project

May 4 2020

amccarth added inline comments to D79269: [NativeSession] Implement NativeSession::findSymbolByAddress..
May 4 2020, 12:24 PM · Restricted Project
amccarth accepted D79028: [DebugInfo][CodeView] Include namespace into emitted globals.
May 4 2020, 8:32 AM · Restricted Project

May 1 2020

amccarth created D79274: Fix template class debug info for Visual Studio visualizers.
May 1 2020, 5:13 PM · Restricted Project
amccarth accepted D79223: Fix pr31836 on Windows too, and correctly handle repeated separators..

OK as is, but please consider re-using llvm::sys::path to distinguish separators.

May 1 2020, 5:10 PM · Restricted Project

Apr 30 2020

amccarth added a comment to D76869: [Clang] Restore replace_path_prefix instead of startswith .

Thanks for including me in this discussion.

Apr 30 2020, 5:11 PM · Restricted Project, Restricted Project
amccarth accepted D79028: [DebugInfo][CodeView] Include namespace into emitted globals.

Apologies for the delay. I thought I had already responded to this one.

Apr 30 2020, 3:06 PM · Restricted Project
amccarth added a comment to D78896: [Support] Add file lock/unlock functions.

Thanks for the timeout fix on Windows. I'm happy now, but I'm not qualified to approve other parts of this patch.

Apr 30 2020, 10:08 AM · Restricted Project
amccarth added a comment to D76863: Fix SelectionDAG Graph Printing on Windows.

Apologies for the drive-by questions.

Apr 30 2020, 10:08 AM · Restricted Project
amccarth added a comment to D76869: [Clang] Restore replace_path_prefix instead of startswith .

The llvm::sys::Path functions are designed to deal with paths that are either Posix-style or (a subset of) Windows styles. We also have the virtual file system (VFS), specifically the redirecting file system, which can truly be hybrids, mixing Posix with Windows styles in a single path.

Apr 30 2020, 9:02 AM · Restricted Project, Restricted Project

Apr 29 2020

amccarth added a comment to D78896: [Support] Add file lock/unlock functions.

If the goal is to synchronize writes to a highly contended log file, would it be better (and feasible) to have the individual threads/processes write timestamped output to separate streams that can be merged after-the-fact?

Apr 29 2020, 9:06 AM · Restricted Project
amccarth added a comment to D79005: [LLD][COFF] Move debug info for thread-local variables into PDB global stream.

This seems to make sense.

Apr 29 2020, 9:06 AM · Restricted Project

Apr 27 2020

amccarth added a comment to D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms..

Personally, I don't consider those to be useless casts. They appropriately ensure that the types match between the arguments and the format specifiers, rather than relying on underlying details of what SIZE_T expands to for a given target. I would expect these to at least use the warn_format_conversion_argument_type_mismatch_confusion diagnostic rather than be silenced entirely.

Apr 27 2020, 10:11 AM · Restricted Project

Apr 20 2020

amccarth accepted D78128: Implement some functions in NativeSession..

Thanks. I appreciate the separation of concerns with regard to finding and loading of the PDB.

Apr 20 2020, 4:50 PM · Restricted Project

Apr 16 2020

amccarth accepted D78249: Reland "[codeview] Reference types in type parent scopes".

Thanks!

Apr 16 2020, 11:44 AM · Restricted Project