Page MenuHomePhabricator

jcai19 (Jian Cai)
User

Projects

User does not belong to any projects.

User Details

User Since
May 15 2019, 3:25 PM (88 w, 3 d)

Recent Activity

Thu, Jan 14

jcai19 closed D91460: [AsmParser] make .ascii support spaces as separators.

The test failure turned out to be a false positive. Relanded the change on https://github.com/llvm/llvm-project/commit/9dfeec853008109b1cbe926c22675c96226040d9

Thu, Jan 14, 5:55 PM · Restricted Project
jcai19 committed rG9dfeec853008: Reland "[AsmParser] make .ascii support spaces as separators" (authored by jcai19).
Reland "[AsmParser] make .ascii support spaces as separators"
Thu, Jan 14, 5:53 PM
jcai19 added a comment to D93881: [llvm-objcopy] preserve file ownership when overwritten.

I have mentioned that the GNU objcopy/strip smart_rename still has vulnerability and the owner preservation does not work for non-root (CAP_CHOWN) users.

Thu, Jan 14, 5:27 PM · Restricted Project
jcai19 added a comment to D93881: [llvm-objcopy] preserve file ownership when overwritten.

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

Thu, Jan 14, 3:15 PM · Restricted Project

Wed, Jan 13

jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

Which GDB tests? How are they broken? I'm extremely surprised by the given this patch implements what binutils has done for a very long time (only .asciz/.string have changed behaviour in binutils, and those are not modified by this patch here so should not have regressions).

Wed, Jan 13, 3:04 PM · Restricted Project
jcai19 added a reverting change for rGe0963ae274be: [AsmParser] make .ascii support spaces as separators: rG82c4153e66fa: Revert "[AsmParser] make .ascii support spaces as separators".
Wed, Jan 13, 2:39 PM
jcai19 committed rG82c4153e66fa: Revert "[AsmParser] make .ascii support spaces as separators" (authored by jcai19).
Revert "[AsmParser] make .ascii support spaces as separators"
Wed, Jan 13, 2:39 PM
jcai19 added a reverting change for D91460: [AsmParser] make .ascii support spaces as separators: rG82c4153e66fa: Revert "[AsmParser] make .ascii support spaces as separators".
Wed, Jan 13, 2:38 PM · Restricted Project

Fri, Jan 8

jcai19 added a comment to D93881: [llvm-objcopy] preserve file ownership when overwritten.

From a user standpoint, I agree that "objcopy foo.o" should not be changing the ownership of a file, regardless of the implementation (which, in the case of llvm-objcopy, is getting it "wrong" compared to GNU objcopy, because we use temporary file and rename it at the end, without fixing ownership). Sounds like there's agreement there.

Fri, Jan 8, 11:56 AM · Restricted Project
jcai19 added a comment to D93881: [llvm-objcopy] preserve file ownership when overwritten.

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

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

Wed, Jan 6

jcai19 added inline comments to D93881: [llvm-objcopy] preserve file ownership when overwritten.
Wed, Jan 6, 2:49 PM · Restricted Project
jcai19 updated the diff for D93881: [llvm-objcopy] preserve file ownership when overwritten.

Address some comments.

Wed, Jan 6, 2:48 PM · Restricted Project

Tue, Jan 5

jcai19 published D93881: [llvm-objcopy] preserve file ownership when overwritten for review.
Tue, Jan 5, 9:27 PM · Restricted Project

Dec 20 2020

jcai19 committed rGe0963ae274be: [AsmParser] make .ascii support spaces as separators (authored by jcai19).
[AsmParser] make .ascii support spaces as separators
Dec 20 2020, 10:45 PM
jcai19 closed D91460: [AsmParser] make .ascii support spaces as separators.
Dec 20 2020, 10:45 PM · Restricted Project

Dec 17 2020

jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

Looks good to me, let's see what the others think.

Dec 17 2020, 2:59 PM · Restricted Project
jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Dec 17 2020, 2:41 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Update the test case.

Dec 17 2020, 2:11 PM · Restricted Project
jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

Just want to follow up and see if anyone might have any thoughts on the current implementation of this patch. Otherwise, is it good to submit the patch? @jrtc27 @rnk @nickdesaulniers @MaskRay

Dec 17 2020, 1:37 PM · Restricted Project

Dec 4 2020

jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Updated a test case.

Dec 4 2020, 3:54 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Update the syntax in the comment.

Dec 4 2020, 3:19 PM · Restricted Project
jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Dec 4 2020, 3:18 PM · Restricted Project
jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Dec 4 2020, 3:03 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Address @jrtc27's comment. paseOp function now skips all the spaces between string arguments until it hits any comma or end of statement. For now we will limit the support to .ascii only.

Dec 4 2020, 2:23 PM · Restricted Project

Dec 3 2020

jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Dec 3 2020, 5:32 PM · Restricted Project
jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Dec 3 2020, 4:24 PM · Restricted Project

Nov 30 2020

jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Address comments.

Nov 30 2020, 3:42 PM · Restricted Project
jcai19 retitled D91460: [AsmParser] make .ascii support spaces as separators from [AsmParser] make .ascii/.asciz/.string support spaces as separators to [AsmParser] make .ascii support spaces as separators.
Nov 30 2020, 2:53 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Since GNU assembler changed how it interpreted using space as separators in .asciz and .string in https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=3d955acb36f483c05724181da5ffba46b1303c43, limit this patch to .ascii only as matching the new GAS behavior in the two directives may cause subtle issues when building existing code that assumes the old behavior with LLVM's integrated assembler.

Nov 30 2020, 2:52 PM · Restricted Project

Nov 20 2020

jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Nov 20 2020, 12:45 PM · Restricted Project
jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Nov 20 2020, 12:25 PM · Restricted Project

Nov 19 2020

jcai19 retitled D91460: [AsmParser] make .ascii support spaces as separators from [AsmParser] make .ascii/.asciz/.string support spaces as the separator to [AsmParser] make .ascii/.asciz/.string support spaces as separators.
Nov 19 2020, 6:07 PM · Restricted Project
jcai19 retitled D91460: [AsmParser] make .ascii support spaces as separators from [AsmParser] make .ascii/.asciz/.string support multiple strings to [AsmParser] make .ascii/.asciz/.string support spaces as the separator.
Nov 19 2020, 6:07 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Simplify test cases as @MaskRay suggested.

Nov 19 2020, 5:08 PM · Restricted Project
jcai19 added inline comments to D91460: [AsmParser] make .ascii support spaces as separators.
Nov 19 2020, 3:50 PM · Restricted Project

Nov 17 2020

jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

I think rejecting .asciz "a" "b" may be fine for us since there is no real world usage.

Then I am not sure we need a new method parseManyWithOptionalComma. It can be specialized.

Nov 17 2020, 4:12 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Thanks all for the comments. From what I read so far, we agree thatt there is a real need to implement this feature in LLVM so add more test cases based on the comments.

Nov 17 2020, 12:15 PM · Restricted Project

Nov 16 2020

jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

https://github.com/ClangBuiltLinux/linux/blob/f01c30de86f1047e9bae1b1b1417b0ce8dcd15b1/arch/arm/probes/kprobes/test-core.h#L114-L116 makes it sounds like .ascii and .asciz work differently in this regard?

Nov 16 2020, 1:18 PM · Restricted Project

Nov 13 2020

jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

In which case that's confusing and likely to lead to bugs if people make use of the preprocessor in the hopes of concatenating strings but end up with NUL bytes being inserted contrary to what there would be in C. Can we not just fix the assembly to use commas rather than this weird syntax that seems to be a special case for .asciz? You can't write .word 2 2, only .word 2, 2, so why do string directives really need special treatment?

Nov 13 2020, 9:29 PM · Restricted Project
jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

In which case I assume .asciz "foo" "bar" is equivalent to .asciz "foobar" rather than .asciz "foo\0bar" (same for .string), just like C string concatenation, and hence why both syntaxes exist?

Nov 13 2020, 9:03 PM · Restricted Project
jcai19 updated the diff for D91460: [AsmParser] make .ascii support spaces as separators.

Update commit message and add a test case.

Nov 13 2020, 8:57 PM · Restricted Project
jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

It appears that I misunderstood the existing code. It actually supports multiple string arguments, but only when commas are used between the arguments. GAS supports commas or spaces as the separator (or when both are used in the same call such as .ascii "aa", "bb" "cc ). Will refactor my patch and update the commit message.

Nov 13 2020, 3:21 PM · Restricted Project
jcai19 added a comment to D91460: [AsmParser] make .ascii support spaces as separators.

This needs tests, both to show it accepts multiple strings and to show for each of the three directives that it produces the right output (i.e. is equivalent to multiple individual directives).

Nov 13 2020, 1:59 PM · Restricted Project
jcai19 added reviewers for D91460: [AsmParser] make .ascii support spaces as separators: MaskRay, peter.smith, ostannard, efriedma, rnk, craig.topper, reames.
Nov 13 2020, 1:48 PM · Restricted Project
jcai19 added a reviewer for D91460: [AsmParser] make .ascii support spaces as separators: nickdesaulniers.
Nov 13 2020, 1:42 PM · Restricted Project
jcai19 requested review of D91460: [AsmParser] make .ascii support spaces as separators.
Nov 13 2020, 1:42 PM · Restricted Project

Sep 9 2020

jcai19 committed rG415a4fbea7c1: [MC] Resolve the difference of symbols in consecutive MCDataFragements (authored by jcai19).
[MC] Resolve the difference of symbols in consecutive MCDataFragements
Sep 9 2020, 12:40 PM
jcai19 closed D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.
Sep 9 2020, 12:39 PM · Restricted Project

Sep 2 2020

jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Moved the definition of SubsectionNumber to avoid padding bytes. Also updated the comments.

Sep 2 2020, 4:09 PM · Restricted Project
jcai19 added a comment to D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

at the very least, if we want to keep it, it should be reordered as my comment says, and don't repeat the name of the variable

Hi @MaskRay, thanks for the comment. By reordering, do you mean moving the definition of SubsectionNumber right after LayoutOrder? I do not understand what difference that would make, would you care to explain a little bit more? Also what by not repeating the name of the variable, I assume you were referring to the comment right above its definition?

Sep 2 2020, 4:02 PM · Restricted Project
jcai19 added a comment to D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

at the very least, if we want to keep it, it should be reordered as my comment says, and don't repeat the name of the variable

Sep 2 2020, 3:44 PM · Restricted Project
jcai19 updated subscribers of D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

It looks like there may have been unresolved comments from @MaskRay . I'm also curious whether @jyknight or @psmith had parting thoughts? (@jcai19 maybe wait a week for their feedback?)

Sep 2 2020, 1:37 PM · Restricted Project

Sep 1 2020

jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Address @psmith's comments.

Sep 1 2020, 4:47 PM · Restricted Project

Aug 31 2020

jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Address more comments.

Aug 31 2020, 7:22 PM · Restricted Project

Aug 28 2020

jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Address @nickdesaulniers' comments.

Aug 28 2020, 3:57 PM · Restricted Project

Aug 27 2020

jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Combine test cases into one file.

Aug 27 2020, 3:36 PM · Restricted Project

Aug 24 2020

jcai19 added a comment to D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.
Aug 24 2020, 6:55 PM · Restricted Project
jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Address some of @MaskRay's comments.

Aug 24 2020, 6:47 PM · Restricted Project

Aug 18 2020

jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Set the subsection number for fragments in subsecitons.

Aug 18 2020, 6:05 PM · Restricted Project
jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

(1) Fixed a bug while rebasing the patch last time. The failed unit tests dropped to 2 after the fix.
(2) Updated the 2 failed unit tests.

Aug 18 2020, 5:48 PM · Restricted Project

Aug 17 2020

jcai19 retitled D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements from [MC] Parse .if conditions with symbols in consecutive MCDataFragements to [MC] Resolve the difference of symbols in consecutive MCDataFragements.
Aug 17 2020, 4:19 PM · Restricted Project
jcai19 updated the diff for D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

Limit the scope to fragments in the same subsection (fragments not in any subsections are placed in subsection 0). Fragments are inserted into each subsection in order and the difference of their offsets should be fixed once they are inserted. This is experimental as it breaks many tests. Will address the test failure if this idea is proven correct.

Aug 17 2020, 4:13 PM · Restricted Project

Aug 11 2020

jcai19 added a comment to D85620: [AARCH64] [MC] add memtag as an alias of mte architecture extension.

Thanks all!

Aug 11 2020, 1:30 PM · Restricted Project
jcai19 committed rG277873ce0f48: [AARCH64] [MC] add memtag as an alias of mte architecture extension (authored by jcai19).
[AARCH64] [MC] add memtag as an alias of mte architecture extension
Aug 11 2020, 1:29 PM
jcai19 closed D85620: [AARCH64] [MC] add memtag as an alias of mte architecture extension.
Aug 11 2020, 1:29 PM · Restricted Project

Aug 10 2020

jcai19 updated subscribers of D85620: [AARCH64] [MC] add memtag as an alias of mte architecture extension.
Aug 10 2020, 11:25 AM · Restricted Project

Aug 9 2020

jcai19 added reviewers for D85620: [AARCH64] [MC] add memtag as an alias of mte architecture extension: olista01, t.p.northover, DavidSpickett, dnsampaio, nickdesaulniers.
Aug 9 2020, 2:45 PM · Restricted Project
jcai19 requested review of D85620: [AARCH64] [MC] add memtag as an alias of mte architecture extension.
Aug 9 2020, 2:41 PM · Restricted Project

Aug 5 2020

jcai19 added a comment to D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements.

@jyknight while working on a different issue, I happened to take a look at the implementation of subsection (MCSection::getSubsectionInsertionPoint) and from what I understood, fragments in subsections were always placed after fragments not in any subsection., and fragments in the same subsection (or not in any subsection) were always inserted in order and separate from fragments in other subsections. Is it safe to assume what between two fragments either not in any subsections or in the same subsection will be remain unchanged once they are inserted to the fragment list of a section? If so, can we resolve differences of two labels in fragments with such restriction?

Aug 5 2020, 4:58 PM · Restricted Project

Aug 3 2020

jcai19 committed rGc6334db577e7: [X86] support .nops directive (authored by jcai19).
[X86] support .nops directive
Aug 3 2020, 11:51 AM
jcai19 closed D82826: [X86] support .nops directive.
Aug 3 2020, 11:51 AM · Restricted Project
jcai19 added a comment to D82826: [X86] support .nops directive.

LGTM

Aug 3 2020, 10:49 AM · Restricted Project

Jul 31 2020

jcai19 updated the diff for D82826: [X86] support .nops directive.

Renamed a variable name and addressed comments. @craig.topper Please let me know if it looks any better. And thanks for enabling multibyte-NOPs in 64-bit mode.

Jul 31 2020, 11:53 AM · Restricted Project

Jul 23 2020

jcai19 added inline comments to D82826: [X86] support .nops directive.
Jul 23 2020, 3:02 PM · Restricted Project

Jul 15 2020

jcai19 added a comment to D82826: [X86] support .nops directive.

I think I have a very old (I emphasized it again in another comment) comment which isn't addressed (https://reviews.llvm.org/D82826#2126406 )
We should align with GNU as on the largest operand .nops supports.

Jul 15 2020, 3:22 PM · Restricted Project
jcai19 added a comment to D82826: [X86] support .nops directive.

@reames @craig.topper Hi just want to double check if there are any additional issues I should address. Thanks.

Jul 15 2020, 3:02 PM · Restricted Project

Jul 7 2020

jcai19 added inline comments to D82826: [X86] support .nops directive.
Jul 7 2020, 3:44 PM · Restricted Project
jcai19 added inline comments to D82826: [X86] support .nops directive.
Jul 7 2020, 3:36 PM · Restricted Project
jcai19 updated the diff for D82826: [X86] support .nops directive.

Fixed an assertion message.

Jul 7 2020, 3:28 PM · Restricted Project
jcai19 updated the diff for D82826: [X86] support .nops directive.

Address some of @reames's concerns, and rename variables to avoid confusion.

Jul 7 2020, 2:55 PM · Restricted Project
jcai19 added inline comments to D82826: [X86] support .nops directive.
Jul 7 2020, 2:39 PM · Restricted Project
jcai19 added inline comments to D82826: [X86] support .nops directive.
Jul 7 2020, 1:43 PM · Restricted Project

Jul 6 2020

jcai19 updated the diff for D82826: [X86] support .nops directive.

Issuing multiple-byte NOP for 32-bit mode broke many tests and probably worth a separate patch itself. So this patch will keep the behavior on 32-bit mode unchange for now. This built and passed all the tests.

Jul 6 2020, 3:47 PM · Restricted Project

Jul 1 2020

jcai19 updated the diff for D82826: [X86] support .nops directive.

Refactor X86AsmBackend::getMaximumNopSize based on @craig.topper's comment.

Jul 1 2020, 6:23 PM · Restricted Project
jcai19 added a comment to D82826: [X86] support .nops directive.

I'm having trouble posting comments inline; it seems fabricator won't allow me to "save" comments.

Is it too painful to make the member of MCNopsFragment uint64_t? a la MCStreamer::emitFill

Jul 1 2020, 6:23 PM · Restricted Project
jcai19 updated the diff for D82826: [X86] support .nops directive.

(1) This is a temporary implementation as it breaks the following tests:

Jul 1 2020, 5:18 PM · Restricted Project
jcai19 added a comment to D82826: [X86] support .nops directive.

is the 32-bit different the 2 byte case?

Jul 1 2020, 1:32 PM · Restricted Project
jcai19 added a comment to D82826: [X86] support .nops directive.

Thanks! Checking if the CPU is 64-bit along FeatureNOPL seemed to work. Will verify more. Also I'd like to point out the difference of 32-bit CPUs.

Jul 1 2020, 11:54 AM · Restricted Project

Jun 30 2020

jcai19 added a comment to D82826: [X86] support .nops directive.

@MaskRay @nickdesaulniers Thanks for the comments. Will refactor the code once we decide which way to go with the implementation.

Jun 30 2020, 10:43 PM · Restricted Project
jcai19 added a comment to D82826: [X86] support .nops directive.

@craig.topper I tried to add a new Fragment type and use X86AsmBackend::writeNopData to insert NOP instructions. It however changed the outcome of the test cases I used, as X86AsmBackend::writeNopData only generatssingle-byte nop instructions if X86::FeatureNOPL feature bit is not true, while the maxLongNopLength in the current implementation (from X86MCInstLower) allows long nop instructions up to 10 bytes on a 64-bit processor. Which one is preferred? Also, which target triple should I use to test multiple-byte nop instructions? Thanks.

Jun 30 2020, 10:43 PM · Restricted Project

Jun 29 2020

jcai19 added a comment to D82826: [X86] support .nops directive.

Would it be possible to emit a new Fragment type and leverage the existing NOP emission code in X86AsmBackend.cpp. The code here appears to be copied from X86MCInstLower. Which would mean we would now have 3 places that do almost the same thing.

Jun 29 2020, 6:34 PM · Restricted Project
jcai19 added reviewers for D82826: [X86] support .nops directive: craig.topper, reames, rnk, sanjoy, MaskRay.
Jun 29 2020, 5:48 PM · Restricted Project
jcai19 added a reviewer for D82826: [X86] support .nops directive: aganea.
Jun 29 2020, 5:27 PM · Restricted Project
jcai19 created D82826: [X86] support .nops directive.
Jun 29 2020, 5:27 PM · Restricted Project

Jun 9 2020

jcai19 added a comment to D80028: [AArch64] Support expression results as immediate values in mov.

I ended up pushing a fix for the racy tests in 7e6f891df85

Jun 9 2020, 10:25 AM · Restricted Project

Jun 8 2020

jcai19 committed rG0e1accd0f726: [AArch64] Support expression results as immediate values in mov (authored by jcai19).
[AArch64] Support expression results as immediate values in mov
Jun 8 2020, 6:17 PM
jcai19 closed D80028: [AArch64] Support expression results as immediate values in mov.
Jun 8 2020, 6:17 PM · Restricted Project
jcai19 added a comment to D80028: [AArch64] Support expression results as immediate values in mov.

LGTM

Jun 8 2020, 6:16 PM · Restricted Project
jcai19 committed rG4db2b7024868: Add a flag to debug automatic variable initialization (authored by jcai19).
Add a flag to debug automatic variable initialization
Jun 8 2020, 12:46 PM
jcai19 closed D77168: Add a flag to debug automatic variable initialization.
Jun 8 2020, 12:46 PM · Restricted Project