Skip to content

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

Closed
yf225 wants to merge 32 commits intopytorch:masterfrom
yf225:ubsancpy
Closed

[Ready for Review] Turn on UBSAN in the OSS build#8813
yf225 wants to merge 32 commits intopytorch:masterfrom
yf225:ubsancpy

Conversation

@yf225
Copy link
Contributor

@yf225 yf225 commented Jun 22, 2018

Copy of #8802

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

cpuhrsch commented Jun 22, 2018

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

(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

cpuhrsch commented Jun 24, 2018

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

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

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

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
#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

@ssnl ssnl ssnl 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

+2 more reviewers

@apaszke apaszke apaszke left review comments

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

Reviewers whose approvals may not affect merge requirements

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