Page MenuHomePhabricator

andrewng (Andrew Ng)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Thu, Sep 26

andrewng added a comment to D68087: [ELF] Set SectionBase::partition in processSectionCommands.

Apart from the fix for the test, LGTM.

Thu, Sep 26, 9:03 AM · Restricted Project
andrewng added a comment to D67504: [ELF] Make MergeInputSection merging aware of output sections.

This change has caused some regressions in some of our local testing. I've filed the bug PR43461 (https://bugs.llvm.org/show_bug.cgi?id=43461) for the issue.

Thu, Sep 26, 5:29 AM · Restricted Project

Jul 15 2019

Herald added a project to D54963: [lit] Pass more environment variables through on Windows: Restricted Project.
Jul 15 2019, 6:34 AM · Restricted Project

Jul 5 2019

andrewng added a comment to D64130: [LLD][ELF] - Linkerscript: add a support for expressions for section's filling.

This change breaks if you add FILL(0x10101010) before *(.aaa) in fill.test:

# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/fill.s -o %t.o
# RUN: ld.lld -o %t --script %s %t.o
# RUN: llvm-objdump -s %t | FileCheck %s
Jul 5 2019, 8:32 AM · Restricted Project

Jul 1 2019

andrewng committed rGd74f2d0a8609: [benchmark] Disable CMake get_git_version (authored by andrewng).
[benchmark] Disable CMake get_git_version
Jul 1 2019, 4:11 AM
andrewng closed D63925: [benchmark] Disable CMake get_git_version.
Jul 1 2019, 4:10 AM · Restricted Project

Jun 28 2019

andrewng abandoned D63657: [benchmark] Change GetGitVersion to only check "dirty" when a tag is found.

Abandoning in favour of D63925.

Jun 28 2019, 4:29 AM · Restricted Project
andrewng created D63925: [benchmark] Disable CMake get_git_version.
Jun 28 2019, 4:29 AM · Restricted Project
andrewng added a comment to D63657: [benchmark] Change GetGitVersion to only check "dirty" when a tag is found.

Ping.

Jun 28 2019, 2:55 AM · Restricted Project

Jun 22 2019

andrewng added a comment to D63657: [benchmark] Change GetGitVersion to only check "dirty" when a tag is found.

(that being said this entire script is kinda bogus, in this sense of integration into other build, the version tag makes no sense since it will be a llvm-one)

Jun 22 2019, 11:58 AM · Restricted Project
andrewng updated the diff for D63657: [benchmark] Change GetGitVersion to only check "dirty" when a tag is found.

Simplified as suggested by reviewer.

Jun 22 2019, 11:57 AM · Restricted Project
andrewng added a comment to D63657: [benchmark] Change GetGitVersion to only check "dirty" when a tag is found.
  1. Can you just use something along the lines of git describe --always --dirty to avoid the update-index/diff-index?
Jun 22 2019, 11:47 AM · Restricted Project

Jun 21 2019

andrewng created D63657: [benchmark] Change GetGitVersion to only check "dirty" when a tag is found.
Jun 21 2019, 10:17 AM · Restricted Project

Jun 3 2019

andrewng added a comment to D62768: [ELF] Don't create an output section named `/DISCARD/` if it is assigned to the special phdr `NONE`.

If I read correctly, this is to assign /DISCARD to the memory region rodata and expect .rodata to inherit the memory region?

Sorry I didn't follow the discussion in D61186 and didn't notice D61251. It seems @grimar and @peter.smith were fine with D61251 (this patch is essentially D61251). @andrewng if you are ok with letting this somewhat contrived example fail, we can probably still mark this as resolved (wontfix).

Jun 3 2019, 8:12 AM · Restricted Project
andrewng added a comment to D62768: [ELF] Don't create an output section named `/DISCARD/` if it is assigned to the special phdr `NONE`.

George's comment reminds me why we didn't take this approach in the first place, as it doesn't work correctly for the following test case:

Jun 3 2019, 3:00 AM · Restricted Project

May 10 2019

andrewng added a comment to D61599: [Support] Reverted r357058 and r351916, MemoryBlock class now exposes its allocated size instead of requested size..

Applied Clang Format to the changed files. Renamed bufferSize -> BufSize to match naming convention..

May 10 2019, 5:24 AM · Restricted Project

May 9 2019

andrewng added a comment to D61599: [Support] Reverted r357058 and r351916, MemoryBlock class now exposes its allocated size instead of requested size..

Some of the changes in "lib/Support/Unix/Memory.inc" don't appear to have been clang formatted.

May 9 2019, 8:47 AM · Restricted Project

May 8 2019

andrewng added a comment to D61599: [Support] Reverted r357058 and r351916, MemoryBlock class now exposes its allocated size instead of requested size..

I don't think this is the right solution. MemoryBlock's purpose is to track the allocated memory. What the client does with that memory (including subdividing it) is their business. The client already knows the number of bytes they're requesting, so the fix for fragmentation in SectionMemoryManager should be limited to fixes in SectionMemoryManager.

Yes I agree it would be better if the Size member of MemoryBlock just held the allocated size. This would mean that r357058 and r351916 (and perhaps others) should be reverted.

May 8 2019, 2:59 AM · Restricted Project

Apr 30 2019

andrewng added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

Submitted bug https://bugs.llvm.org/show_bug.cgi?id=41673 regarding the issue that the /DISCARD/ section is in the output.

Apr 30 2019, 7:57 AM · Restricted Project
andrewng committed rG24896d304df9: [LLD][ELF] /DISCARD/ output sections should not be orphans (authored by andrewng).
[LLD][ELF] /DISCARD/ output sections should not be orphans
Apr 30 2019, 7:30 AM
andrewng added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

Can I land this patch?

Apr 30 2019, 6:43 AM · Restricted Project
andrewng committed rG0f4c58f6f408: [LLD][ELF] Fix getRankProximity to "ignore" not live sections (authored by andrewng).
[LLD][ELF] Fix getRankProximity to "ignore" not live sections
Apr 30 2019, 5:26 AM
andrewng added inline comments to D61197: [LLD][ELF] Fix getRankProximity to "ignore" not live sections.
Apr 30 2019, 3:24 AM · Restricted Project

Apr 29 2019

andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

D61197 seems to have addressed the issue, D61186 made no difference.

Apr 29 2019, 2:53 PM · Restricted Project
andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

This change broke our kernel build, I filed a bug PR41653 which has more details, but it seems like that with this change, linker produces wrong layout. Can we please revert this change until we figure out how to address this issue?

Apr 29 2019, 1:21 PM · Restricted Project
andrewng added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

Just to clarify my understanding, are you referring to (https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/Using_ld_the_GNU_Linker/sections.html#OUTPUT-SECTION-PHDR)

If a section is assigned to one or more segments, then all subsequent allocated sections will be assigned to those segments as well, unless they use an explicitly :phdr modifier. You can use :NONE to tell the linker to not put the section in any segment at all.

Something like:

/DISCARD/ : { *(.comment) } :my_phdr
 .name : { *(.name) }
...

Where we'd want .name to to be assigned to my_phdr and any following OutputSections? I can see that this is how ld.bfd and ld.gold behave, although as you say this is somewhat of a contrived example.

Apr 29 2019, 9:04 AM · Restricted Project
andrewng added a comment to D61197: [LLD][ELF] Fix getRankProximity to "ignore" not live sections.

This is how this function used to behave (see r316307), when findOrphanPos ignored non-live sections.

Apr 29 2019, 8:16 AM · Restricted Project
andrewng added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).

Apr 29 2019, 6:45 AM · Restricted Project
andrewng added inline comments to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.
Apr 29 2019, 6:10 AM · Restricted Project
andrewng updated the diff for D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

Simplified test as suggested in review.

Apr 29 2019, 6:09 AM · Restricted Project
andrewng added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

I think it is 2 lines of code change actually :) I believe the much more correct way to fix it is this: https://reviews.llvm.org/D61251
With this approach there is no /DISCARD/ in the output.
(note, I did not change the original test case there).

Apr 29 2019, 2:57 AM · Restricted Project

Apr 26 2019

andrewng created D61197: [LLD][ELF] Fix getRankProximity to "ignore" not live sections.
Apr 26 2019, 9:04 AM · Restricted Project
andrewng added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

From what I see in the output for this case LLD creates an /DISCARD/ output section.
This does not look correct to me, because /DISCARD/ is kind of key word, but not a section name I believe.
Also, bfd does not do that.

So I think issue first of all is that we are trying to create output section with this name, it should never be in the output.

Apr 26 2019, 8:20 AM · Restricted Project
andrewng created D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.
Apr 26 2019, 6:48 AM · Restricted Project

Apr 23 2019

andrewng committed rGccba42c7eb3c: [ELF] Change default output section type to SHT_PROGBITS (authored by andrewng).
[ELF] Change default output section type to SHT_PROGBITS
Apr 23 2019, 5:37 AM
andrewng committed rG98c858a23be1: [ELF] Change findOrphanPos to only consider live sections (authored by andrewng).
[ELF] Change findOrphanPos to only consider live sections
Apr 23 2019, 5:18 AM

Apr 18 2019

andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

IIUC, you are fixing an issue that a segment whose size is 0 doesn't have a file offset that is congruent modulo page size. Is this correct?

The file offset for an empty segment is not actually significant; we can just set a dummy value. Have you considered that?

Apr 18 2019, 12:12 AM · Restricted Project

Apr 15 2019

andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

Is this patch acceptable or do we need to start looking for an alternative (and most probably more complex solution) to the issue described above?

Apr 15 2019, 5:07 AM · Restricted Project

Apr 4 2019

andrewng added a comment to D60273: [ELF] Change findOrphanPos to only consider live sections.

This would be the follow up patch to D60131 as a fix for the orphan PHDR test failure.

Apr 4 2019, 8:55 AM · Restricted Project
andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

The follow up patch to fix the orphan PHDR test is D60273.

Apr 4 2019, 8:55 AM · Restricted Project
andrewng created D60273: [ELF] Change findOrphanPos to only consider live sections.
Apr 4 2019, 8:51 AM · Restricted Project

Apr 3 2019

andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

This seems like the wrong fix to me. Shouldn't we be fixing the alignment bug with respect to segments not changing things like this?

If I recall correctly I made a change to make NOBITS the default because some sections nthat could have been NOBITS we're winding up as PROGBITS. I don't remember the exact details. I'm fairly sure this change regresses that point. You've changedna bunch of test from expecting NOBITS to expecting PROGBITS but that seems unjustified.

Apr 3 2019, 7:50 AM · Restricted Project
andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

I am not sure we have the practice to disable the test cases. What do you think about uploading the fix for orphan behavior
in a separate change rebased on this patch too, so that these 2 patches can be reviewed together?
(in that case, having XFAIL here is fine I think).

Apr 3 2019, 3:15 AM · Restricted Project
andrewng added inline comments to D60131: [ELF] Change default output section type to SHT_PROGBITS.
Apr 3 2019, 3:09 AM · Restricted Project
andrewng updated the diff for D60131: [ELF] Change default output section type to SHT_PROGBITS.

Updated to address review comments.

Apr 3 2019, 3:09 AM · Restricted Project
andrewng updated subscribers of D60131: [ELF] Change default output section type to SHT_PROGBITS.
Apr 3 2019, 2:28 AM · Restricted Project

Apr 2 2019

andrewng added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

The default was SHT_PROGBITS in the past, and was changed to SHT_NOBITS to "reduce file size". However, it's not clear that it actually reduces output file size, but only reduces the value of file size in the output. In any case, it does cause the alignment problem described and shown in the symbol-only-align.test test.

Apr 2 2019, 8:27 AM · Restricted Project
andrewng created D60131: [ELF] Change default output section type to SHT_PROGBITS.
Apr 2 2019, 8:20 AM · Restricted Project

Mar 27 2019

andrewng committed rGe6b6ab2c6685: [LLD] Restore tests that use "-" as output (authored by andrewng).
[LLD] Restore tests that use "-" as output
Mar 27 2019, 8:30 AM
andrewng committed rG2fc69abf5be8: [Support] MemoryBlock size should reflect the requested size (authored by andrewng).
[Support] MemoryBlock size should reflect the requested size
Mar 27 2019, 3:26 AM

Mar 26 2019

andrewng added a comment to D59824: [LLD] Restore tests that use "-" as output.

I think there are more tests that should use -o - in wasm/target-feature-none.yaml, but otherwise the wasm changes lgtm.

Mar 26 2019, 11:05 AM · Restricted Project
andrewng added a comment to D59663: [Support] MemoryBlock size should reflect the requested size.

I've added D59824 to restore testing and usage of output to "-".

Mar 26 2019, 9:22 AM · Restricted Project
andrewng created D59824: [LLD] Restore tests that use "-" as output.
Mar 26 2019, 9:22 AM · Restricted Project

Mar 21 2019

andrewng added a comment to D59663: [Support] MemoryBlock size should reflect the requested size.

The stdout.s test you added in fact failed on Windows because this change was missing, and @thakis disabled it in rL351949. So, I think we can just re-enable stdout.s to test this.

Mar 21 2019, 5:10 PM · Restricted Project
andrewng added a reviewer for D59663: [Support] MemoryBlock size should reflect the requested size: rnk.
Mar 21 2019, 12:29 PM · Restricted Project
andrewng created D59663: [Support] MemoryBlock size should reflect the requested size.
Mar 21 2019, 12:20 PM · Restricted Project

Feb 26 2019

andrewng committed rGf38b00532118: [TableGen] Make OpcodeMappings sort comparator deterministic NFCI (authored by andrewng).
[TableGen] Make OpcodeMappings sort comparator deterministic NFCI
Feb 26 2019, 10:51 AM
andrewng created D58687: [TableGen] Make OpcodeMappings sort comparator deterministic NFCI.
Feb 26 2019, 10:22 AM · Restricted Project
andrewng committed rG301f304949ad: [clang-format] SpaceBeforeParens for lambda expressions (authored by andrewng).
[clang-format] SpaceBeforeParens for lambda expressions
Feb 26 2019, 6:34 AM

Feb 25 2019

andrewng added a reviewer for D58241: [clang-format] SpaceBeforeParens for lambda expressions: benhamilton.
Feb 25 2019, 7:28 AM · Restricted Project

Feb 21 2019

andrewng added a comment to D58241: [clang-format] SpaceBeforeParens for lambda expressions.

Ping...

Feb 21 2019, 6:03 AM · Restricted Project

Feb 14 2019

andrewng created D58241: [clang-format] SpaceBeforeParens for lambda expressions.
Feb 14 2019, 9:25 AM · Restricted Project
andrewng committed rGd27cf27eb1fe: [Support] Fix TempFile::discard to not leave behind temporary files (authored by andrewng).
[Support] Fix TempFile::discard to not leave behind temporary files
Feb 14 2019, 3:10 AM

Feb 12 2019

andrewng updated the diff for D57960: [Support] Fix TempFile::discard to not leave behind temporary files.

Updated the patch based on review suggestion.

Feb 12 2019, 4:01 AM · Restricted Project

Feb 11 2019

andrewng updated the diff for D57960: [Support] Fix TempFile::discard to not leave behind temporary files.

As suggested, I have switched back to what was my original change to fix this issue.

Feb 11 2019, 2:42 AM · Restricted Project

Feb 8 2019

andrewng added a comment to D57960: [Support] Fix TempFile::discard to not leave behind temporary files.

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

Feb 8 2019, 11:28 AM · Restricted Project
andrewng created D57960: [Support] Fix TempFile::discard to not leave behind temporary files.
Feb 8 2019, 9:45 AM · Restricted Project

Nov 14 2018

andrewng updated subscribers of D54422: [ELF] - Do not ICF two sections with different output sections when using linker scripts.
Nov 14 2018, 4:31 AM

Jul 20 2018

andrewng updated the diff for D49557: [ELF] Fix handling of FDE negative relative PC addr.

Update to the eh-frame-negative-pcrel-sdata8.s test.

Jul 20 2018, 10:18 AM
andrewng updated the diff for D49557: [ELF] Fix handling of FDE negative relative PC addr.

Updated testing as per review comments.

Jul 20 2018, 8:47 AM
andrewng added a comment to D49557: [ELF] Fix handling of FDE negative relative PC addr.

Thanks! How did yoy find this?

Jul 20 2018, 2:47 AM

Jul 19 2018

andrewng created D49557: [ELF] Fix handling of FDE negative relative PC addr.
Jul 19 2018, 10:25 AM

Apr 20 2018

andrewng created D45871: [DebugInfo] Use WithColor for more debug line warnings.
Apr 20 2018, 4:06 AM
andrewng created D45869: [DebugInfo] Fix for split dwarf test on Windows (NFC).
Apr 20 2018, 3:31 AM

Mar 27 2018

andrewng abandoned D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.

Abandoned in favour of D44923.

Mar 27 2018, 8:10 AM
andrewng updated the diff for D44923: [ELF] Disable ICF for synthetic sections.

Updated to address review comments.

Mar 27 2018, 6:49 AM
andrewng added a comment to D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.

2.8%. Somehow clang-fsds got 2.39% faster.

That speedup is interesting. I don't have a good theory to explain it though.

Even if this patch is somewhat performance-neutral, we still should avoid extra memory allocation if we can. Let's try ignoring synthetic sections in ICF. That's I think better.

Mar 27 2018, 6:07 AM
andrewng created D44923: [ELF] Disable ICF for synthetic sections.
Mar 27 2018, 6:04 AM

Mar 26 2018

andrewng added a comment to D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.

How much did Scylla get slower?

Maybe the best option is just disabling ICF of synthetic sections?

That sounds like a better way of fixing the issue.

Mar 26 2018, 3:07 PM
andrewng updated the diff for D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.

Updated to fix issue with clang compilation.

Mar 26 2018, 9:40 AM
andrewng updated the diff for D44682: [ELF] Fix X86 & X86_64 PLT retpoline padding.

Removed assertions and tidied up white space a little.

Mar 26 2018, 9:25 AM
andrewng updated the diff for D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.

Updated test to address review comments.

Mar 26 2018, 9:14 AM
andrewng added a comment to D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.

Seems reasonable. I will just do a quick check on the performance impact.

Mar 26 2018, 8:56 AM
andrewng created D44898: [ELF] Fix incorrect deduplication of MergeSyntheticSection's.
Mar 26 2018, 8:31 AM

Mar 25 2018

andrewng added a comment to D44682: [ELF] Fix X86 & X86_64 PLT retpoline padding.

I think the PLT writing should handle the padding itself. It certainly feels more correct. It looks though D44775 is heading in that direction too.

Mar 25 2018, 10:15 AM

Mar 21 2018

andrewng added a comment to D44682: [ELF] Fix X86 & X86_64 PLT retpoline padding.

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

Mar 21 2018, 3:27 AM

Mar 20 2018

andrewng updated subscribers of D44682: [ELF] Fix X86 & X86_64 PLT retpoline padding.
Mar 20 2018, 8:27 AM
andrewng created D44682: [ELF] Fix X86 & X86_64 PLT retpoline padding.
Mar 20 2018, 8:21 AM

Mar 15 2018

andrewng added a comment to D44168: [ELF] Add .eh_frame pieces to map file.

Looking at the EhFrameSection writing and finalize code, it would seem that
some kind of alignment is being applied. This aligning is also the reason
why for x86_64 the mapping of CIE/FDE's to the output is so fragmented,
because all input we have seen is 4-byte aligned, but LLD uses 8-byte
alignment for the output.

Mar 15 2018, 8:08 AM

Mar 9 2018

andrewng added a comment to D44168: [ELF] Add .eh_frame pieces to map file.

Apart from the typo and the value for the alignment, LGTM.

Mar 9 2018, 6:32 AM
andrewng abandoned D42960: [ELF] Add .eh_frame pieces to map file.

Abandoning this revision in favour of D44168.

Mar 9 2018, 6:24 AM

Mar 8 2018

andrewng added a comment to D42960: [ELF] Add .eh_frame pieces to map file.

There are also some slight differences in the output between this implementation and the one in D44168. This patch does not output the "+0x0" for a zero offset and outputs the input section's alignment for the "Align" column. Which style of output is preferred?

Mar 8 2018, 2:22 AM

Mar 7 2018

andrewng updated the diff for D42960: [ELF] Add .eh_frame pieces to map file.

Updated to address review comments.

Mar 7 2018, 5:24 AM

Mar 6 2018

andrewng added a comment to D44168: [ELF] Add .eh_frame pieces to map file.

I agree that this change is simpler, but it also doesn't produce the same output as D42960. Part of the reason for the "complexity" in my change in D42960 was to try to prevent the map file output from getting too big/verbose.

Mar 6 2018, 11:14 AM
andrewng updated the diff for D42960: [ELF] Add .eh_frame pieces to map file.

Updated to address comments.

Mar 6 2018, 9:50 AM
andrewng retitled D42960: [ELF] Add .eh_frame pieces to map file from [ELF] DEMO: Example for adding .eh_frame pieces to map file to [ELF] Add .eh_frame pieces to map file.
Mar 6 2018, 9:42 AM

Mar 1 2018

andrewng updated the diff for D42960: [ELF] Add .eh_frame pieces to map file.

Rebased and updated with minor change to make the output offset hex rather than decimal. Modified existing map file test to demonstrate the proposed .eh_frame output.

Mar 1 2018, 7:43 AM
andrewng commandeered D42960: [ELF] Add .eh_frame pieces to map file.
Mar 1 2018, 7:39 AM

Nov 22 2017

andrewng added a comment to D40366: [Support][Parallel] ThreadPoolExecutor fixes for Windows.

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Nov 22 2017, 11:08 AM
andrewng created D40366: [Support][Parallel] ThreadPoolExecutor fixes for Windows.
Nov 22 2017, 10:35 AM