Skip to content

[Ready for Review] Turn on UBSAN in the OSS build #8813

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 32 commits into from
Closed

[Ready for Review] Turn on UBSAN in the OSS build #8813

wants to merge 32 commits into from

Conversation

yf225
Copy link
Contributor

@yf225 yf225 commented Jun 22, 2018

Copy of #8802

@yf225 yf225 requested review from ezyang and cpuhrsch June 22, 2018 23:25
@cpuhrsch
Copy link
Contributor

Still seems to be getting stuck in tp

23:28:38 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/lib/jenkins/workspace/third_party/protobuf/src/google/protobuf/io/printer.cc:353:12 in

@ssnl
Copy link
Collaborator

ssnl commented Jun 23, 2018

Maybe writing a special list file will work? https://clang.llvm.org/docs/SanitizerSpecialCaseList.html

ssnl
ssnl reviewed Jun 24, 2018
View reviewed changes
.jenkins/pytorch/test.sh Outdated
@@ -29,8 +29,9 @@ if [[ "$BUILD_ENVIRONMENT" == *asan* ]]; then
ulimit -s 81920

(cd test && python -c "import torch")
echo "The next two invocations are expected to crash; if they don't that means ASAN is misconfigured"
echo "The next two invocations are expected to crash; if they don't that means ASAN/UBSAN is misconfigured"

This comment was marked as off-topic.

Sign in to view

@cpuhrsch
Copy link
Contributor

Still got stuck in third part

1:47:00 /var/lib/jenkins/workspace/third_party/protobuf/src/google/protobuf/io/printer.cc:353:12: runtime error: null pointer passed as argument 1, which is declared to never be null
01:47:00 /usr/include/string.h:43:28: note: nonnull attribute specified here
01:47:00 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/lib/jenkins/workspace/third_party/protobuf/src/google/protobuf/io/printer.cc:353:12 in 
01:47:00 third_party/onnx/CMakeFiles/gen_onnx_proto.dir/build.make:61: recipe for target 'third_party/onnx/onnx/onnx_onnx_torch.pb.cc' failed
01:47:00 make[2]: *** [third_party/onnx/onnx/onnx_onnx_torch.pb.cc] Error 1
01:47:00 make[1]: *** [third_party/onnx/CMakeFiles/gen_onnx_proto.dir/all] Error 2

@cpuhrsch
Copy link
Contributor

cpuhrsch commented Jun 24, 2018 •
edited
Loading

Maybe you need a recursive specifier? It could be that third_party/* only limits itself to files directly under third_party

@yf225
Copy link
Contributor Author

yf225 commented Jun 27, 2018

@cpuhrsch @ssnl I tried many variations but wasn't able to get the special casing to work. The current way of removing the UBSAN flag from protobuf might be the way to go for now.

This should be able to bring OSS CI to parity with Sandcastle's test toolchain.

@cpuhrsch
Copy link
Contributor

cpuhrsch commented Jun 27, 2018 •
edited
Loading 

00:42:22 test_acos (__main__.TestAutograd) ... /var/lib/jenkins/workspace/aten/src/ATen/cpu/vec256/vec256_base.h:179:31: runtime error: division by zero

We have tests that do this on purpose to check for "bad" return values (nan etc.).

@yf225 yf225 changed the title Turn on UBSAN in the OSS build [WIP] Turn on UBSAN in the OSS build Jun 27, 2018
cpuhrsch
cpuhrsch reviewed Jun 27, 2018
View reviewed changes
test/test_autograd.py Outdated
@@ -14,7 +14,7 @@
from torch.autograd.function import once_differentiable
from torch.autograd.profiler import profile
from common import TEST_MKL, TestCase, run_tests, skipIfNoLapack, \
suppress_warnings, skipIfNoZeroSize
suppress_warnings, skipIfNoZeroSize, TEST_WITH_ASAN

This comment was marked as off-topic.

Sign in to view

@ezyang
Copy link
Contributor

ezyang commented Jun 27, 2018

@pytorchbot retest this please

@ezyang
Copy link
Contributor

ezyang commented Jun 28, 2018

Woohoo!

ezyang
ezyang reviewed Jun 28, 2018
View reviewed changes
.jenkins/pytorch/test.sh Outdated
(cd test && ! python -c "import torch; torch._C._crash_if_csrc_asan(3)")
(cd test && ! python -c "import torch; torch._C._crash_if_aten_asan(3)")
echo "The next three invocations are expected to crash; if they don't that means ASAN/UBSAN is misconfigured"
# DEBUG: temporarily disable ASAN/UBSAN checks

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Jun 28, 2018
View reviewed changes
torch/csrc/Module.cpp Outdated
@@ -126,8 +127,15 @@ static PyObject * THPModule_crashIfCsrcASAN(PyObject *module, PyObject *arg) {
return PyLong_FromLong(x[0]);
}

static PyObject * THPModule_crashIfCsrcUBSAN(PyObject *module, PyObject *arg) {
#ifndef _WIN32
__builtin_unreachable();

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang requested changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

Need to turn the smoketests back on.

@cpuhrsch cpuhrsch mentioned this pull request Jun 28, 2018
Closed
@yf225 yf225 force-pushed the ubsancpy branch 3 times, most recently from 68400a3 to 3d0220e Compare June 29, 2018 00:58
@yf225 yf225 changed the title [WIP] Turn on UBSAN in the OSS build [Ready for Review] Turn on UBSAN in the OSS build Jun 29, 2018
ezyang
ezyang reviewed Jun 29, 2018
View reviewed changes
aten/src/ATen/cpu/vec256/vec256_base.h Outdated
@@ -12,6 +12,12 @@
#define __at_align32__
#endif

#if defined(__clang__)

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Jul 5, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yf225
Copy link
Contributor Author

yf225 commented Jul 5, 2018

@pytorchbot retest this please

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 6, 2018
@facebook-github-bot
Turn on UBSAN in the OSS build (#8813)
2bc5b93 
Summary:
Copy of pytorch/pytorch#8802
Closes pytorch/pytorch#8813

Differential Revision: D8707364

Pulled By: yf225

fbshipit-source-id: bc201980b50e9fb44c42a17f898b50d3558fc417
zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 13, 2018
@facebook-github-bot
Turn on UBSAN in the OSS build (#8813)
e241491 
Summary:
Copy of pytorch/pytorch#8802
Closes pytorch/pytorch#8813

Differential Revision: D8707364

Pulled By: yf225

fbshipit-source-id: bc201980b50e9fb44c42a17f898b50d3558fc417
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Turn on UBSAN in the OSS build (pytorch#8813)
53b97f3 
Summary:
Copy of pytorch#8802
Closes pytorch#8813

Differential Revision: D8707364

Pulled By: yf225

fbshipit-source-id: bc201980b50e9fb44c42a17f898b50d3558fc417
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezyang ezyang ezyang approved these changes

@cpuhrsch cpuhrsch cpuhrsch left review comments

@apaszke apaszke apaszke left review comments

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yf225 @cpuhrsch @ssnl @ezyang @apaszke @facebook-github-bot