Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added address sanitization feature for translating ML models to LLVM modules #4676

Merged
merged 18 commits into from
Feb 11, 2021

Conversation

inho9606
Copy link
Contributor

@inho9606 inho9606 commented Feb 1, 2021

  1. Added an option, -iree-llvm-address-sanitizer. Users can turn it on and it injects ASAN to binary of the module.
  2. If ASAN is used on compilation, it changes a bit to true in flat buffer to mark it is compiled with ASAN enabled. IREE runtime can read the bit before running the module. And, it will make an error message if runtime does not have ASAN feature when running address sanitized module.
  3. To test prototype of address sanitizer, I wrote a simple testcase trying to access out of index using torch_index_select. When I executed the module, it displayed address sanitizer error message.
  4. To test micro changes #2, I built two IREE, one is with -DIREE_ENABLE_ASAN_ON option, and another one is without that option. And I checked the one without ASAN option showed IREE_STATUS_UNAVAILABLE error when running a module compiled with ASAN Another one with ASAN option gracefully ran the module.
    I hope this works..

@google-cla google-cla bot added the cla: yes label Feb 1, 2021
@hanhanW hanhanW self-requested a review February 1, 2021 14:18
@hanhanW
Copy link
Contributor

hanhanW commented Feb 1, 2021

I think dylib_executable.cc was moved to https://github.com/google/iree/blob/main/iree/hal/local/loaders/legacy_library_loader.cc

@benvanik is it a right file to modify?

@benvanik
Copy link
Collaborator

benvanik commented Feb 1, 2021

I think dylib_executable.cc was moved to https://github.com/google/iree/blob/main/iree/hal/local/loaders/legacy_library_loader.cc

@benvanik is it a right file to modify?

yep! it'll be replaced with system_library_loader soonish (#3580) but anything you do in there I'll port over so ignore it for now.

iree/compiler/Dialect/HAL/Target/LLVM/LLVMIRPasses.cpp Outdated Show resolved Hide resolved
iree/compiler/Dialect/HAL/Target/LLVM/LLVMAOTTarget.cpp Outdated Show resolved Hide resolved
build_tools/cmake/iree_copts.cmake Outdated Show resolved Hide resolved
iree/schemas/dylib_executable_def.fbs Outdated Show resolved Hide resolved
iree/compiler/Dialect/HAL/Target/LLVM/LLVMTargetOptions.h Outdated Show resolved Hide resolved
iree/compiler/Dialect/HAL/Target/LLVM/LLVMTargetOptions.h Outdated Show resolved Hide resolved
iree/compiler/Dialect/HAL/Target/LLVM/LLVMIRPasses.cpp Outdated Show resolved Hide resolved
iree/compiler/Dialect/HAL/Target/LLVM/LLVMIRPasses.cpp Outdated Show resolved Hide resolved
@hanhanW
Copy link
Contributor

hanhanW commented Feb 3, 2021

As the offline discussion, please try to use the enum type from schema, and rebase to the upstream/main. Thanks!

iree/compiler/Dialect/HAL/Target/LLVM/LLVMTargetOptions.h Outdated Show resolved Hide resolved
iree/compiler/Dialect/HAL/Target/LLVM/LLVMTargetOptions.h Outdated Show resolved Hide resolved
iree/hal/local/loaders/legacy_library_loader.cc Outdated Show resolved Hide resolved
iree/hal/local/loaders/legacy_library_loader.cc Outdated Show resolved Hide resolved
iree/hal/local/loaders/legacy_library_loader.cc Outdated Show resolved Hide resolved
iree/schemas/dylib_executable_def.fbs Outdated Show resolved Hide resolved
@benvanik
Copy link
Collaborator

benvanik commented Feb 5, 2021

Great progress!

@benvanik
Copy link
Collaborator

benvanik commented Feb 5, 2021

and sorry for the delay - when you push new changes you can click the refresh button here to re-request reviews:
image
(otherwise the PR still looks like you haven't addressed feedback so people are unlikely to look again)

@hanhanW
Copy link
Contributor

hanhanW commented Feb 5, 2021

and sorry for the delay - when you push new changes you can click the refresh button here to re-request reviews:
image
(otherwise the PR still looks like you haven't addressed feedback so people are unlikely to look again)

@inho9606 To request a fresh review from a reviewer, in the sidebar of the Conversation tab, click the second icon from right. It will show "Re-request review" if you move the curser on the icon.

iree/compiler/Dialect/HAL/Target/LLVM/LLVMTargetOptions.h Outdated Show resolved Hide resolved
iree/hal/local/loaders/legacy_library_loader.cc Outdated Show resolved Hide resolved
iree/schemas/dylib_executable_def.fbs Outdated Show resolved Hide resolved
iree/schemas/dylib_executable_def.fbs Outdated Show resolved Hide resolved
@inho9606 inho9606 requested a review from hanhanW February 8, 2021 03:45
@inho9606 inho9606 requested a review from hanhanW February 8, 2021 11:15
Copy link
Contributor

@hanhanW hanhanW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

iree/hal/local/loaders/legacy_library_loader.cc Outdated Show resolved Hide resolved
@inho9606 inho9606 requested a review from hanhanW February 9, 2021 01:23
@stellaraccident
Copy link
Collaborator

Hi - is there anything I can help with to get this landed? (it would be super useful in debugging workflows)

@inho9606
Copy link
Contributor Author

Hi - is there anything I can help with to get this landed? (it would be super useful in debugging workflows)

I'm just waiting for merge. Please let me know if I need something to update more fore merge.
Thanks

@stellaraccident
Copy link
Collaborator

I'm just waiting for merge. Please let me know if I need something to update more fore merge.
Thanks

It's still showing as a couple of nit comments unresolved. Did you have any updates to push or is this final?

@inho9606
Copy link
Contributor Author

inho9606 commented Feb 11, 2021

I'm just waiting for merge. Please let me know if I need something to update more fore merge.
Thanks

It's still showing as a couple of nit comments unresolved. Did you have any updates to push or is this final?
I think this is final, and at least Hanhan approved it.

@hanhanW
Copy link
Contributor

hanhanW commented Feb 11, 2021

This needs an approval from @benvanik because he requested changes before.

@hanhanW hanhanW merged commit e775b54 into iree-org:main Feb 11, 2021
@inho9606 inho9606 deleted the flatBuffer branch February 11, 2021 07:36
benvanik added a commit that referenced this pull request Feb 11, 2021
benvanik added a commit that referenced this pull request Feb 11, 2021
benvanik added a commit that referenced this pull request Feb 11, 2021
This was referenced Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants