Page MenuHomePhabricator

andrewng (Andrew Ng)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 2 2017, 6:37 AM (185 w, 6 d)

Recent Activity

Today

andrewng added inline comments to D88151: [LLD][ELF] Fix inconsistencies with ICF equality class.
Wed, Sep 23, 9:13 AM
andrewng added a comment to D88151: [LLD][ELF] Fix inconsistencies with ICF equality class.

I may be missing something about how the ICF code works, but whilst looking at a separate ICF performance issue, I noticed these oddities in the code. I haven't actually seen any issues myself but reading through the code, it feels like there could be issues. Although I suspect they would be highly unlikely or perhaps not possible in actual practice.

Wed, Sep 23, 7:59 AM
andrewng updated subscribers of D88151: [LLD][ELF] Fix inconsistencies with ICF equality class.
Wed, Sep 23, 7:51 AM
andrewng requested review of D88151: [LLD][ELF] Fix inconsistencies with ICF equality class.
Wed, Sep 23, 7:50 AM

Wed, Sep 16

andrewng committed rG77152a6b7ac0: [LLD][ELF] Optimize linker script filename glob pattern matching NFC (authored by andrewng).
[LLD][ELF] Optimize linker script filename glob pattern matching NFC
Wed, Sep 16, 2:28 AM
andrewng committed rG6040e2a6d97d: [Support] Add GlobPattern::isTrivialMatchAll() (authored by andrewng).
[Support] Add GlobPattern::isTrivialMatchAll()
Wed, Sep 16, 2:28 AM
andrewng closed D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.
Wed, Sep 16, 2:27 AM · Restricted Project
andrewng closed D87468: [Support] Add GlobPattern::isTrivialMatchAll().
Wed, Sep 16, 2:27 AM · Restricted Project

Tue, Sep 15

andrewng added inline comments to D87468: [Support] Add GlobPattern::isTrivialMatchAll().
Tue, Sep 15, 10:55 AM · Restricted Project
andrewng updated the diff for D87468: [Support] Add GlobPattern::isTrivialMatchAll().

Update for review suggestion.

Tue, Sep 15, 10:55 AM · Restricted Project
andrewng added inline comments to D87468: [Support] Add GlobPattern::isTrivialMatchAll().
Tue, Sep 15, 7:15 AM · Restricted Project
andrewng updated the diff for D87468: [Support] Add GlobPattern::isTrivialMatchAll().

Update for review comments.

Tue, Sep 15, 7:06 AM · Restricted Project
andrewng added inline comments to D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.
Tue, Sep 15, 6:30 AM · Restricted Project
andrewng updated the diff for D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.

Update to address review comments.

Tue, Sep 15, 6:12 AM · Restricted Project

Fri, Sep 11

andrewng added a comment to D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.

I think this is fine approach in general. I wonder if it should/could be splitted into 2 patches though:
one for InputSectionDescription and one for SectionPattern?

Fri, Sep 11, 8:01 AM · Restricted Project
andrewng updated the diff for D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.

Update to address review comments and suggestions.

Fri, Sep 11, 7:56 AM · Restricted Project
andrewng added inline comments to D87468: [Support] Add GlobPattern::isTrivialMatchAll().
Fri, Sep 11, 7:52 AM · Restricted Project
andrewng updated the diff for D87468: [Support] Add GlobPattern::isTrivialMatchAll().

Updated to address review comments and suggestions.

Fri, Sep 11, 7:48 AM · Restricted Project

Thu, Sep 10

andrewng added a comment to D87468: [Support] Add GlobPattern::isTrivialMatchAll().

Can you demonstrate how the old code has overhead related to the input?

Thu, Sep 10, 11:17 AM · Restricted Project
andrewng added inline comments to D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.
Thu, Sep 10, 11:01 AM · Restricted Project
andrewng added a comment to D87468: [Support] Add GlobPattern::isTrivialMatchAll().

GlobPattern::matchOne has a fast path for '*'. Isn't it fast enough?

Thu, Sep 10, 10:52 AM · Restricted Project
andrewng added a comment to D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.

This change requires D87468.

Thu, Sep 10, 10:24 AM · Restricted Project
andrewng added a comment to D87468: [Support] Add GlobPattern::isTrivialMatchAll().

This is a prerequisite of D87469.

Thu, Sep 10, 10:23 AM · Restricted Project
andrewng added a reviewer for D87468: [Support] Add GlobPattern::isTrivialMatchAll(): grimar.
Thu, Sep 10, 10:22 AM · Restricted Project
andrewng requested review of D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC.
Thu, Sep 10, 10:20 AM · Restricted Project
andrewng requested review of D87468: [Support] Add GlobPattern::isTrivialMatchAll().
Thu, Sep 10, 10:15 AM · Restricted Project
andrewng added inline comments to D87272: [lld] Buffer writes when composing a single diagnostic.
Thu, Sep 10, 2:25 AM · Restricted Project

Tue, Sep 8

andrewng committed rG863aa0a37bd1: [LLD][ELF] Fix performance of MarkLive::scanEhFrameSection (authored by andrewng).
[LLD][ELF] Fix performance of MarkLive::scanEhFrameSection
Tue, Sep 8, 11:36 AM
andrewng closed D87245: [LLD][ELF] Fix performance of MarkLive::scanEhFrameSection.
Tue, Sep 8, 11:36 AM · Restricted Project
andrewng added a comment to D87245: [LLD][ELF] Fix performance of MarkLive::scanEhFrameSection.

Thanks for noting this issue! Were you profiling ld.lld?

Tue, Sep 8, 1:59 AM · Restricted Project
andrewng updated the diff for D87245: [LLD][ELF] Fix performance of MarkLive::scanEhFrameSection.

Address review comments.

Tue, Sep 8, 1:51 AM · Restricted Project

Mon, Sep 7

andrewng requested review of D87245: [LLD][ELF] Fix performance of MarkLive::scanEhFrameSection.
Mon, Sep 7, 10:09 AM · Restricted Project

Thu, Aug 27

andrewng committed rGd4e2e2852aff: [ELF][test] Add test coverage of TLS to gc-sections.s (authored by andrewng).
[ELF][test] Add test coverage of TLS to gc-sections.s
Thu, Aug 27, 4:30 AM
andrewng closed D86639: [ELF][test] Add test coverage of TLS to gc-sections.s.
Thu, Aug 27, 4:30 AM · Restricted Project

Wed, Aug 26

andrewng requested review of D86639: [ELF][test] Add test coverage of TLS to gc-sections.s.
Wed, Aug 26, 9:46 AM · Restricted Project
andrewng accepted D86564: [Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle().

LGTM with that last one change.

Wed, Aug 26, 7:21 AM · Restricted Project
andrewng added inline comments to D86564: [Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle().
Wed, Aug 26, 6:20 AM · Restricted Project
andrewng added inline comments to D86564: [Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle().
Wed, Aug 26, 6:01 AM · Restricted Project
andrewng added inline comments to D86564: [Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle().
Wed, Aug 26, 2:34 AM · Restricted Project

Jul 29 2020

andrewng committed rG8725a49409c4: [ELF][test] Add test coverage of `__real_` to wrap-plt.s (authored by andrewng).
[ELF][test] Add test coverage of `__real_` to wrap-plt.s
Jul 29 2020, 6:12 AM
andrewng closed D84749: [ELF][test] Add test coverage of `__real_` to wrap-plt.s.
Jul 29 2020, 6:12 AM · Restricted Project

Jul 28 2020

andrewng requested review of D84749: [ELF][test] Add test coverage of `__real_` to wrap-plt.s.
Jul 28 2020, 6:41 AM · Restricted Project

Jul 15 2020

andrewng committed rGf6eb5daa1636: [Support] Fix Windows directory_iterator_construct out of bounds (authored by andrewng).
[Support] Fix Windows directory_iterator_construct out of bounds
Jul 15 2020, 2:19 AM
andrewng closed D83689: [Support] Fix Windows directory_iterator_construct out of bounds.
Jul 15 2020, 2:19 AM · Restricted Project

Jul 13 2020

Herald added a project to D83689: [Support] Fix Windows directory_iterator_construct out of bounds: Restricted Project.
Jul 13 2020, 10:04 AM · Restricted Project

Jul 9 2020

andrewng added a comment to D83321: [Support] Fix utf16 path's index upper bound.

That test is for widenPath and it does cover a similar situation (I know because I wrote it). The code you have changed relates to directory_iterator_construct which does not have coverage of the situation where the UTF16 length is less than the UTF8 length, i.e. the issue that you are fixing.

Jul 9 2020, 6:55 AM · Restricted Project
andrewng added a comment to D83321: [Support] Fix utf16 path's index upper bound.

Full context attached @andrewng .

Jul 9 2020, 3:53 AM · Restricted Project

Jul 8 2020

andrewng added a comment to D83321: [Support] Fix utf16 path's index upper bound.

The usual practice is to include the entire context in the diff. However, the change itself LGTM.

Jul 8 2020, 8:28 AM · Restricted Project

Apr 14 2020

andrewng added a comment to D77750: [ELF][test] Improve reproduce tests and enable for Windows.

Can you take a look, please?

Apr 14 2020, 8:00 AM · Restricted Project

Apr 9 2020

andrewng committed rGd08105482e15: [ELF][test] Improve reproduce tests and enable for Windows (authored by andrewng).
[ELF][test] Improve reproduce tests and enable for Windows
Apr 9 2020, 8:08 AM
andrewng closed D77750: [ELF][test] Improve reproduce tests and enable for Windows.
Apr 9 2020, 8:08 AM · Restricted Project

Apr 8 2020

andrewng created D77750: [ELF][test] Improve reproduce tests and enable for Windows.
Apr 8 2020, 1:02 PM · Restricted Project
andrewng committed rG3db215089f48: [ELF][test] Add reproduce test for dependent libraries (authored by andrewng).
[ELF][test] Add reproduce test for dependent libraries
Apr 8 2020, 5:24 AM
andrewng closed D77659: [ELF][test] Add reproduce test for dependent libraries.
Apr 8 2020, 5:24 AM · Restricted Project
andrewng added inline comments to D77659: [ELF][test] Add reproduce test for dependent libraries.
Apr 8 2020, 4:17 AM · Restricted Project
andrewng updated the diff for D77659: [ELF][test] Add reproduce test for dependent libraries.

Add comment to explain the Windows deficiencies (one of many)...

Apr 8 2020, 3:12 AM · Restricted Project
andrewng added inline comments to D77659: [ELF][test] Add reproduce test for dependent libraries.
Apr 8 2020, 2:39 AM · Restricted Project

Apr 7 2020

andrewng added inline comments to D77659: [ELF][test] Add reproduce test for dependent libraries.
Apr 7 2020, 2:10 PM · Restricted Project
andrewng updated the diff for D77659: [ELF][test] Add reproduce test for dependent libraries.

Updated to address review comments.

Apr 7 2020, 2:10 PM · Restricted Project
andrewng added inline comments to D77659: [ELF][test] Add reproduce test for dependent libraries.
Apr 7 2020, 11:23 AM · Restricted Project
andrewng updated the diff for D77659: [ELF][test] Add reproduce test for dependent libraries.

Updated to address review comments.

Apr 7 2020, 11:23 AM · Restricted Project
andrewng created D77659: [ELF][test] Add reproduce test for dependent libraries.
Apr 7 2020, 9:44 AM · Restricted Project

Apr 6 2020

andrewng added a comment to D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang.

grimar, andrewng: You both have checkout and build on different drives too, yes?

Apr 6 2020, 3:45 AM · Restricted Project, Restricted Project

Apr 3 2020

andrewng added a comment to D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang.

The following patch fixes my issues on Windows, but I haven't tested that it doesn't break anything else:

Apr 3 2020, 5:20 AM · Restricted Project, Restricted Project

Mar 23 2020

andrewng committed rG328b72dd8209: [Support] Fix clang warning in widenPath NFC (authored by andrewng).
[Support] Fix clang warning in widenPath NFC
Mar 23 2020, 12:01 PM
andrewng closed D76544: [Support] Fix clang warning in widenPath NFC.
Mar 23 2020, 12:01 PM · Restricted Project

Mar 21 2020

andrewng created D76544: [Support] Fix clang warning in widenPath NFC.
Mar 21 2020, 5:20 AM · Restricted Project

Mar 19 2020

andrewng committed rGe6f6c551213a: [Support] Improve Windows widenPath and add support for long UNC paths (authored by andrewng).
[Support] Improve Windows widenPath and add support for long UNC paths
Mar 19 2020, 6:27 AM
andrewng closed D75372: [Support] Improve Windows widenPath and add support for long UNC paths.
Mar 19 2020, 6:27 AM · Restricted Project

Mar 16 2020

andrewng added a comment to D75372: [Support] Improve Windows widenPath and add support for long UNC paths.

Ping.

Mar 16 2020, 5:52 AM · Restricted Project

Mar 9 2020

andrewng added a comment to D75372: [Support] Improve Windows widenPath and add support for long UNC paths.

Ping.

Mar 9 2020, 4:48 AM · Restricted Project

Mar 2 2020

andrewng updated the summary of D75372: [Support] Improve Windows widenPath and add support for long UNC paths.
Mar 2 2020, 3:53 AM · Restricted Project
andrewng updated the diff for D75372: [Support] Improve Windows widenPath and add support for long UNC paths.

Updated to address review comments and suggestions.

Mar 2 2020, 3:53 AM · Restricted Project
andrewng added inline comments to D75372: [Support] Improve Windows widenPath and add support for long UNC paths.
Mar 2 2020, 3:53 AM · Restricted Project

Feb 28 2020

andrewng created D75372: [Support] Improve Windows widenPath and add support for long UNC paths.
Feb 28 2020, 10:51 AM · Restricted Project

Feb 14 2020

andrewng added a comment to D74477: [llvm-ar] Simplify Windows comparePaths NFCI.

Just curious: Do we know that the paths stored in the archive are actually UTF-8 and not just MBCS using whatever code page the user had when they created the archive?

Feb 14 2020, 4:34 AM · Restricted Project
andrewng committed rG430fc538e6dc: [llvm-ar] Simplify Windows comparePaths NFCI (authored by andrewng).
[llvm-ar] Simplify Windows comparePaths NFCI
Feb 14 2020, 3:24 AM
andrewng closed D74477: [llvm-ar] Simplify Windows comparePaths NFCI.
Feb 14 2020, 3:24 AM · Restricted Project

Feb 13 2020

andrewng added a comment to D74477: [llvm-ar] Simplify Windows comparePaths NFCI.

In my modified version of widenPath...

I second grimar's request for an associated test that covers this behavior, but are you saying that the issue is your modified version of sys::path::widenPath breaks some llvm-ar tests, and switching to sys::windows::UTF8ToUTF16 will keep them passing? (If so, submitting w/o an additional test sounds fine to me)

Feb 13 2020, 10:43 AM · Restricted Project
andrewng added a comment to D74477: [llvm-ar] Simplify Windows comparePaths NFCI.

Just to add a bit more context for this change, I have a patch in progress to fix Windows UNC path handling in widenPath and that's how I stumbled upon this usage in llvm-ar. In my modified version of widenPath, I use the length of the input as UTF-16 to decide on whether the path needs to be "expanded", which should reduce the scope for problems in this llvm-ar use case. However, it really isn't needed here and hence this patch.

Feb 13 2020, 3:16 AM · Restricted Project
andrewng edited reviewers for D74477: [llvm-ar] Simplify Windows comparePaths NFCI, added: ruiu, mstorsjo; removed: gbreynoo.
Feb 13 2020, 2:58 AM · Restricted Project

Feb 12 2020

andrewng added a comment to D74477: [llvm-ar] Simplify Windows comparePaths NFCI.

This is not necessary for CompareStringOrdinal and could possibly even cause problems.

Perhaps I am not a good person to review this patch. Have a question though: by "cause problems" do you know something that can be wrapped into a test case?

Feb 12 2020, 6:13 AM · Restricted Project
andrewng created D74477: [llvm-ar] Simplify Windows comparePaths NFCI.
Feb 12 2020, 3:56 AM · Restricted Project

Jan 28 2020

andrewng committed rGde2dfc8b203f: [LLD] Avoid exiting with a locked mutex NFC (authored by andrewng).
[LLD] Avoid exiting with a locked mutex NFC
Jan 28 2020, 8:49 AM
andrewng closed D73281: [LLD] Avoid exiting with a locked mutex NFC.
Jan 28 2020, 8:49 AM · Restricted Project

Jan 24 2020

andrewng added a comment to D73281: [LLD] Avoid exiting with a locked mutex NFC.

It is generally not good practice to hold a lock whilst exiting, as this has the potential to cause deadlocks. I don't believe that this situation can occur currently with LLD. However, after some recent observations, it appears that on Windows, in particular when using the static run-time, the only guaranteed way to not intermittently crash on exit is to wait for all threads to terminate (see related review D70447). Doing so would enable this deadlock situation to occur.

Jan 24 2020, 2:51 AM · Restricted Project

Jan 23 2020

andrewng created D73281: [LLD] Avoid exiting with a locked mutex NFC.
Jan 23 2020, 10:45 AM · Restricted Project

Jan 21 2020

andrewng committed rG4e8116f4692e: [ELF] Refactor uses of getInputSections to improve efficiency NFC (authored by andrewng).
[ELF] Refactor uses of getInputSections to improve efficiency NFC
Jan 21 2020, 4:39 AM
andrewng closed D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC.
Jan 21 2020, 4:39 AM · Restricted Project
andrewng updated the diff for D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC.

Updated patch to address review comments.

Jan 21 2020, 2:56 AM · Restricted Project

Jan 20 2020

andrewng added a comment to D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC.

Noticed this again whilst working on D72775. Appears to have a minor performance benefit of ~2% for the same two links with stack sizes mentioned in D72775.

Jan 20 2020, 7:53 AM · Restricted Project
andrewng created D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC.
Jan 20 2020, 7:49 AM · Restricted Project
andrewng added reviewers for D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC: ruiu, MaskRay.
Jan 20 2020, 7:49 AM · Restricted Project

Jan 16 2020

andrewng committed rGd36b2649e5e4: [ELF] Optimization to LinkerScript::computeInputSections NFC (authored by andrewng).
[ELF] Optimization to LinkerScript::computeInputSections NFC
Jan 16 2020, 5:57 AM
andrewng closed D72775: [ELF] Optimization to LinkerScript::computeInputSections NFC.
Jan 16 2020, 5:57 AM · Restricted Project
andrewng added a comment to D72775: [ELF] Optimization to LinkerScript::computeInputSections NFC.

It'd be great if you can provide the exact numbers.

No stack sizesStack SizesOverhead of stack sizes
Link 11.246s2.127s70.71%
Link 1*0.874s0.995s13.84%
Link 22.042s3.885s90.25%
Link 2*1.635s1.933s18.23%

Where * = with this patch
Run with a release build with assertions enabled on Windows 10 with AMD Ryzen 3900X (12C/24T).

Jan 16 2020, 5:55 AM · Restricted Project

Jan 15 2020

andrewng created D72775: [ELF] Optimization to LinkerScript::computeInputSections NFC.
Jan 15 2020, 9:05 AM · Restricted Project

Jan 10 2020

andrewng committed rG564481aebe18: [Support] ThreadPoolExecutor fixes for Windows/MinGW (authored by andrewng).
[Support] ThreadPoolExecutor fixes for Windows/MinGW
Jan 10 2020, 5:06 AM
andrewng closed D70447: [Support] ThreadPoolExecutor fixes for Windows/MinGW.
Jan 10 2020, 5:06 AM · Restricted Project

Jan 6 2020

andrewng updated the diff for D70447: [Support] ThreadPoolExecutor fixes for Windows/MinGW.

Updated to address review comments.

Jan 6 2020, 6:52 AM · Restricted Project