This is an archive of the discontinued LLVM Phabricator instance.

llvm enable sanitizer (RISCV64)
Needs ReviewPublic

Authored by joshua-arch1 on Aug 19 2020, 12:42 AM.

Details

Summary

compiler-rt/

    
* cmake/config-ix.cmake: New options for RISCV64.
* test/asan/CMakeLists.txt: Ditto.

Up till now, only UndefinedBehaviorSanitizer(UBSan) is functionally supported for RISC-V on LLVM. With ASan enabled in this patch, AddressSanitizer(ASan) and KernelAddressSANitizer(KASAN) will be both supported with -DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu" added when running 'cmake'.

Diff Detail

Event Timeline

joshua-arch1 created this revision.Aug 19 2020, 12:42 AM
joshua-arch1 requested review of this revision.Aug 19 2020, 12:42 AM
joshua-arch1 added a reviewer: lenary.
luismarques requested changes to this revision.Aug 19 2020, 2:52 AM
  1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
  2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
  3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
  4. I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.

[1] https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

This revision now requires changes to proceed.Aug 19 2020, 2:52 AM
joshua-arch1 added a comment.EditedAug 19 2020, 6:33 PM
  1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
  2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
  3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
  4. I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.

[1] https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

What do you mean by '3) This patch repeats changes from D86198'? This patch is actually D86198.
Did you mean D84727? For that patch, no one has given an update feedback so far.

joshua-arch1 edited the summary of this revision. (Show Details)
  1. to provide the full context in the patch
  2. to base the patch on a recent LLVM commit
joshua-arch1 edited the summary of this revision. (Show Details)Aug 20 2020, 12:12 AM
  1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
  2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
  3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
  4. I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.

[1] https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

Hi I have updated the patch according to your comments.

Did you mean D84727? For that patch, no one has given an update feedback so far.

Yes, I meant D84727. I don't see why the lack of additional feedback on that patch lead you to submit another patch that also enabled ASan, among other sanitizers.

Hi I have updated the patch according to your comments.

You didn't actually address point 4. The review summary doesn't accurately list the enabled sanitizers and doesn't provide an explanation or evidence for why enabling them is reasonable. For instance, this patch enables DFSan but there is no actual support for RISC-V, and it won't actually compile as is.

Did you mean D84727? For that patch, no one has given an update feedback so far.

Yes, I meant D84727. I don't see why the lack of additional feedback on that patch lead you to submit another patch that also enabled ASan, among other sanitizers.

Hi I have updated the patch according to your comments.

You didn't actually address point 4. The review summary doesn't accurately list the enabled sanitizers and doesn't provide an explanation or evidence for why enabling them is reasonable. For instance, this patch enables DFSan but there is no actual support for RISC-V, and it won't actually compile as is.

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

For D84727 you need to go to the dropdown menu near the bottom that says "Add Action..." and choose "Abandon Revision". To update a patch you use the "Update Diff" link elsewhere on the review page.

joshua-arch1 edited the summary of this revision. (Show Details)

Remove some unnecessary sanitizers and update the summary

This comment was removed by joshua-arch1.
joshua-arch1 edited reviewers, added: asb; removed: alex.Aug 23 2020, 7:01 PM
joshua-arch1 added a comment.EditedAug 27 2020, 11:33 PM

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

For D84727 you need to go to the dropdown menu near the bottom that says "Add Action..." and choose "Abandon Revision". To update a patch you use the "Update Diff" link elsewhere on the review page.

I have updated my patch and summary. Are there any new updates with the patch review?

I have updated my patch and summary. Are there any new updates with the patch review?

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

I have updated my patch and summary. Are there any new updates with the patch review?

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?

It's not that I had trouble compiling tests due to an incorrect configuration (I think), it's that when running the entire set of sanitizer tests I encountered things (e.g. #ifdefs) that weren't yet implemented for RISC-V, and so would fail to compile or run. Have you tried running the entire set of (relevant) sanitizer checks?

joshua-arch1 added a comment.EditedSep 13 2020, 6:35 PM

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?

It's not that I had trouble compiling tests due to an incorrect configuration (I think), it's that when running the entire set of sanitizer tests I encountered things (e.g. #ifdefs) that weren't yet implemented for RISC-V, and so would fail to compile or run. Have you tried running the entire set of (relevant) sanitizer checks?

I think it is necessary to add the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when running the entire set of sanitizer check. Note that currently I have only added support for the address sanitizer.

RISC-V support for ASan has been merged (see D87582 and its predecessors). This patch should probably be abandoned. Thank you.

luismarques resigned from this revision.Nov 5 2020, 1:42 AM
lenary resigned from this revision.Jan 14 2021, 9:49 AM