Page MenuHomePhabricator

skan (Kan Shengchen)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 23 2019, 8:52 PM (82 w, 6 d)

Recent Activity

Aug 26 2020

skan added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

Longer answer:

binutils 2.26 introduced a regression: R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

The original issue is "Copy relocation against protected symbol doesn't work".
I agree with Rich Felker (https://gcc.gnu.org/ml/gcc/2016-04/msg00168.html) and
Cary Coutant (https://sourceware.org/ml/binutils/2016-03/msg00407.html https://gcc.gnu.org/ml/gcc/2016-04/msg00158.html https://gcc.gnu.org/ml/gcc/2016-04/msg00169.html) that we should
keep using direct access against protected symbols and disallow copy relocations against protected symbols.

I appreciate that Cary Coutant and Rafael Ávila de Espíndola added diagnostics to gold and lld, respectively:

@skan Perhaps you are in a good position to change the resolutions to the following issues:)

GCC 5 x86-64 introduced a regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248) I suggested a fix in May, 2019: https://sourceware.org/pipermail/gcc/2019-May/229309.html
i386 was flagged as a reproduce (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012)

__attribute__((visibility("protected"))) int a;
int foo() { return a; } // GCC>=5 uses R_X86_64_GOTPCREL/R_X86_64_REX_GOTPCRELX instead of R_X86_64_PC32

binutils 2.26 introduced a regression R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

Aug 26 2020, 1:35 AM · Restricted Project

Aug 25 2020

skan added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

I think this introduces an old issue when lowering the instruction.

Aug 25 2020, 2:06 AM · Restricted Project
skan updated subscribers of D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.
Aug 25 2020, 1:43 AM · Restricted Project

Jul 9 2020

skan committed rGe59e39b7c409: [MC] Simplify the logic of applying fixup for fragments, NFCI (authored by skan).
[MC] Simplify the logic of applying fixup for fragments, NFCI
Jul 9 2020, 1:39 AM
skan closed D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI.
Jul 9 2020, 1:39 AM · Restricted Project

Jul 8 2020

skan updated the diff for D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI.

Address review comments

Jul 8 2020, 12:16 AM · Restricted Project

Jul 7 2020

skan added inline comments to D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI.
Jul 7 2020, 10:56 PM · Restricted Project
skan added a comment to D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI.

This is: if else if else if -> switch. I think it is just a refactoring and I agree that it is the right thing to do, but I don't find complexity being simplified.

Jul 7 2020, 10:52 PM · Restricted Project
skan added a comment to D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI.

Note that the number of lines actually increases... so I am not sure this is "simplification"

Jul 7 2020, 10:02 PM · Restricted Project
skan added reviewers for D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI: peter.smith, majnemer, MaskRay, rnk, xiangzhangllvm.
Jul 7 2020, 8:50 PM · Restricted Project
skan updated the diff for D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI.

Update comments

Jul 7 2020, 8:33 PM · Restricted Project
Herald added a project to D83366: [MC] Simplify the logic of applying fixup for fragments, NFCI: Restricted Project.
Jul 7 2020, 8:30 PM · Restricted Project

Jun 8 2020

skan committed rG2c63ea6eded3: [TEST] TreeTest.cpp - Add a comma to avoid build error with -werror (authored by skan).
[TEST] TreeTest.cpp - Add a comma to avoid build error with -werror
Jun 8 2020, 8:16 AM
skan closed D81388: [TEST] TreeTest.cpp - Add a comma to avoid build error with -werror.
Jun 8 2020, 8:15 AM · Restricted Project
skan added a comment to D81388: [TEST] TreeTest.cpp - Add a comma to avoid build error with -werror.

Related revision: D80822

Jun 8 2020, 7:38 AM · Restricted Project
skan added reviewers for D81388: [TEST] TreeTest.cpp - Add a comma to avoid build error with -werror: gribozavr, hlopko, eduucaldas.
Jun 8 2020, 7:05 AM · Restricted Project
skan created D81388: [TEST] TreeTest.cpp - Add a comma to avoid build error with -werror.
Jun 8 2020, 7:05 AM · Restricted Project

Jun 3 2020

skan committed rGac47588bc4ff: [Driver] Add negative option for -fkeep-static-consts (authored by skan).
[Driver] Add negative option for -fkeep-static-consts
Jun 3 2020, 12:31 AM

May 27 2020

skan committed rG495444999549: [Driver][X86] Support branch align options with LTO (authored by skan).
[Driver][X86] Support branch align options with LTO
May 27 2020, 10:21 PM
skan closed D80289: [Driver][X86] Support branch align options with LTO.
May 27 2020, 10:20 PM · Restricted Project
skan added inline comments to D80289: [Driver][X86] Support branch align options with LTO.
May 27 2020, 7:07 PM · Restricted Project

May 22 2020

skan added a comment to D80289: [Driver][X86] Support branch align options with LTO.

Ping.

May 22 2020, 11:57 PM · Restricted Project

May 20 2020

skan updated the diff for D80289: [Driver][X86] Support branch align options with LTO.

Refactor addX86AlignBranchArgs to be reused in CommonArgs.cpp

May 20 2020, 10:33 PM · Restricted Project
skan updated the diff for D80289: [Driver][X86] Support branch align options with LTO.

Move tests to x86-malign-branch.c

May 20 2020, 7:53 PM · Restricted Project
skan updated the summary of D80289: [Driver][X86] Support branch align options with LTO.
May 20 2020, 5:23 AM · Restricted Project
skan created D80289: [Driver][X86] Support branch align options with LTO.
May 20 2020, 4:51 AM · Restricted Project

May 12 2020

skan committed rGad60ff70eb56: [NFC] Code cleanup in TargetInfo.cpp (authored by skan).
[NFC] Code cleanup in TargetInfo.cpp
May 12 2020, 11:58 PM

May 8 2020

skan committed rG99ac9ce7016d: [NFC] Clean up in MCObjectStreamer and X86AsmBackend (authored by skan).
[NFC] Clean up in MCObjectStreamer and X86AsmBackend
May 8 2020, 10:00 PM

Apr 26 2020

skan accepted D78776: [llvm-objdump] Print target address with evaluateMemoryOperandAddress().

LGTM

Apr 26 2020, 12:29 AM · Restricted Project

Apr 25 2020

skan added inline comments to D78776: [llvm-objdump] Print target address with evaluateMemoryOperandAddress().
Apr 25 2020, 11:21 PM · Restricted Project
skan added inline comments to D78776: [llvm-objdump] Print target address with evaluateMemoryOperandAddress().
Apr 25 2020, 10:49 PM · Restricted Project
skan added inline comments to D78776: [llvm-objdump] Print target address with evaluateMemoryOperandAddress().
Apr 25 2020, 10:18 PM · Restricted Project
skan added inline comments to D78776: [llvm-objdump] Print target address with evaluateMemoryOperandAddress().
Apr 25 2020, 9:46 PM · Restricted Project
skan accepted D78775: [MC] Fix quadratic behavior in addPendingLabel.

LGTM (with one comment)

Apr 25 2020, 8:10 PM · Restricted Project

Apr 20 2020

skan committed rG8bb059ab6379: [MC][Bugfix] Remove redundant parameter for relaxInstruction (authored by skan).
[MC][Bugfix] Remove redundant parameter for relaxInstruction
Apr 20 2020, 8:37 PM
skan committed rGc031378ce01b: [MC][NFC] Use camelCase style for functions in MCObjectStreamer (authored by skan).
[MC][NFC] Use camelCase style for functions in MCObjectStreamer
Apr 20 2020, 8:36 PM
skan closed D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.
Apr 20 2020, 8:36 PM · Restricted Project
skan updated the diff for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.

Rebase

Apr 20 2020, 8:04 PM · Restricted Project
skan committed rGf0019d4ff29c: [MC][NFC] Use camelCase style for function EmitInstToData (authored by skan).
[MC][NFC] Use camelCase style for function EmitInstToData
Apr 20 2020, 7:00 PM
skan accepted D78526: [MC][PGO][PGSO] Cleanup unused MBFI in AsmPrinter.

LGTM

Apr 20 2020, 7:00 PM · Restricted Project

Apr 19 2020

skan updated the diff for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.

Address review comments

Apr 19 2020, 7:45 PM · Restricted Project
skan committed rGb78c3c89c266: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part III) (authored by skan).
[X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part III)
Apr 19 2020, 7:13 PM
skan closed D78419: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part III).
Apr 19 2020, 7:13 PM · Restricted Project

Apr 18 2020

skan updated the diff for D78419: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part III).

Address review comments

Apr 18 2020, 11:09 PM · Restricted Project
skan added reviewers for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction: enderby, rtaylor, colinl.
Apr 18 2020, 4:50 AM · Restricted Project
skan added a reviewer for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction: luismarques.
Apr 18 2020, 4:18 AM · Restricted Project
skan updated the diff for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.

Address review comments (only test file changes)

Apr 18 2020, 3:13 AM · Restricted Project
skan added a reviewer for D78419: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part III): MaskRay.
Apr 18 2020, 2:40 AM · Restricted Project
skan created D78419: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part III).
Apr 18 2020, 2:40 AM · Restricted Project

Apr 17 2020

skan committed rG0d3149f43173: [MC][X86] Disable branch align in non-text section (authored by skan).
[MC][X86] Disable branch align in non-text section
Apr 17 2020, 11:58 PM
skan closed D77971: [MC][X86] Disable branch align in non-text section.
Apr 17 2020, 11:58 PM · Restricted Project
skan added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.

Apr 17 2020, 11:57 PM · Restricted Project
skan added inline comments to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.
Apr 17 2020, 11:57 PM · Restricted Project
skan added a reviewer for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction: asb.
Apr 17 2020, 11:57 PM · Restricted Project
skan updated the summary of D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.
Apr 17 2020, 11:07 PM · Restricted Project
skan updated the diff for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.

Add test for RISCV to prevent future regression

Apr 17 2020, 9:29 PM · Restricted Project
skan added a comment to D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.

Feel free to add yourself as a reviewer if you'd like to.

Apr 17 2020, 7:09 PM · Restricted Project
skan added a comment to D77971: [MC][X86] Disable branch align in non-text section.

@MaskRay Is this acceptable?

Apr 17 2020, 7:00 AM · Restricted Project
skan added reviewers for D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction: Razer6, MaskRay, jyknight.
Apr 17 2020, 6:59 AM · Restricted Project
skan updated the summary of D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.
Apr 17 2020, 6:59 AM · Restricted Project
skan created D78364: [MC][Bugfix] Remove redundant parameter for relaxInstruction.
Apr 17 2020, 6:59 AM · Restricted Project
skan added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

Apr 17 2020, 6:27 AM · Restricted Project
skan committed rGc82faea9fb58: Recommit [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter… (authored by skan).
Recommit [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter…
Apr 17 2020, 4:50 AM
skan added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

Apr 17 2020, 2:40 AM · Restricted Project

Apr 16 2020

skan committed rGc5fa0a4d4b85: Temporaily revert [X86][MC][NFC] Reduce the parameters of functions in… (authored by skan).
Temporaily revert [X86][MC][NFC] Reduce the parameters of functions in…
Apr 16 2020, 11:14 PM
skan added a reverting change for rG3017580c7961: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II): rGc5fa0a4d4b85: Temporaily revert [X86][MC][NFC] Reduce the parameters of functions in….
Apr 16 2020, 11:14 PM
skan committed rG3017580c7961: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II) (authored by skan).
[X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II)
Apr 16 2020, 10:42 PM
skan closed D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 10:42 PM · Restricted Project
skan added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated.

Apr 16 2020, 10:10 PM · Restricted Project
skan added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Could you provide a new fail case that can be reproduced?

Apr 16 2020, 8:33 PM · Restricted Project
skan updated the summary of D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 8:33 PM · Restricted Project
skan updated the diff for D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).

Address review comments

Apr 16 2020, 8:33 PM · Restricted Project
skan added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Apr 16 2020, 7:29 PM · Restricted Project
skan updated the diff for D77971: [MC][X86] Disable branch align in non-text section.

Address review comments

Apr 16 2020, 6:57 PM · Restricted Project
skan updated subscribers of D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 1:33 AM · Restricted Project
skan added inline comments to D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 12:54 AM · Restricted Project
skan updated the diff for D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).

Address part of review comments
Check the REX is not zero before emitting it

Apr 16 2020, 12:47 AM · Restricted Project
skan added inline comments to D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 12:47 AM · Restricted Project
skan added reviewers for D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II): craig.topper, pengfei.
Apr 16 2020, 12:47 AM · Restricted Project
skan added inline comments to D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 12:47 AM · Restricted Project
skan updated the diff for D77971: [MC][X86] Disable branch align in non-text section.

Address review comments

Apr 16 2020, 12:47 AM · Restricted Project
skan created D78276: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II).
Apr 16 2020, 12:02 AM · Restricted Project

Apr 15 2020

skan committed rG71303b753c88: [X86] Add interface X86II::isPseudo (authored by skan).
[X86] Add interface X86II::isPseudo
Apr 15 2020, 10:02 PM
skan added a comment to D77971: [MC][X86] Disable branch align in non-text section.

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

I asked some experts about this, and got the answer that putting instructions in non-executable section is legal and their encoding values will be treated as data. Do you have other concern about this patch?

Can you add some information to the description? Ideally there should be an example that instructions in a non-executable section are useful.

Personally I like this resolution because that means we don't need to extend D78138 (bss like sections) to non-executable sections.

Apr 15 2020, 10:01 PM · Restricted Project
skan updated the summary of D77971: [MC][X86] Disable branch align in non-text section.
Apr 15 2020, 9:17 PM · Restricted Project
skan committed rG7aaaea5acd09: [X86][MC][NFC] Code cleanup in X86MCCodeEmitter (authored by skan).
[X86][MC][NFC] Code cleanup in X86MCCodeEmitter
Apr 15 2020, 8:33 PM
skan committed rG6c66bb393e1c: [X86][MC][NFC] Refine code in X86MCCodeEmitter (authored by skan).
[X86][MC][NFC] Refine code in X86MCCodeEmitter
Apr 15 2020, 7:53 PM
skan added a comment to D77971: [MC][X86] Disable branch align in non-text section.

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

Apr 15 2020, 7:52 PM · Restricted Project
skan committed rG322ac2e9173a: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I) (authored by skan).
[X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I)
Apr 15 2020, 7:20 PM
skan closed D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I).
Apr 15 2020, 7:20 PM · Restricted Project
skan accepted D78138: [MC][COFF][ELF] Reject instructions in IMAGE_SCN_CNT_UNINITIALIZED_DATA/SHT_NOBITS sections.

LGTM

Apr 15 2020, 6:46 PM · Restricted Project
skan added a comment to D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I).

It's better to add NFC in the title.
The type of TSFlags is uint64_t instead of const uint64_t, is it possible this value been changed during function call?

Apr 15 2020, 2:41 AM · Restricted Project
skan retitled D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I) from [X86][MC] Reduce the parameters of functions in X86MCCodeEmitter(Part I) to [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I).
Apr 15 2020, 2:41 AM · Restricted Project
skan committed rG22e919ca6141: [NFC][test] Mark the section which contains instructions executable (authored by skan).
[NFC][test] Mark the section which contains instructions executable
Apr 15 2020, 1:36 AM
skan added a reviewer for D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I): pengfei.
Apr 15 2020, 1:36 AM · Restricted Project
skan added inline comments to D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I).
Apr 15 2020, 1:36 AM · Restricted Project
skan updated the diff for D77971: [MC][X86] Disable branch align in non-text section.

Rebase

Apr 15 2020, 1:36 AM · Restricted Project
skan added a comment to D77971: [MC][X86] Disable branch align in non-text section.
Apr 15 2020, 1:36 AM · Restricted Project
skan added reviewers for D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I): craig.topper, MaskRay.
Apr 15 2020, 1:02 AM · Restricted Project
skan created D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I).
Apr 15 2020, 12:30 AM · Restricted Project