Assert that kernels are called with the right signature by smessmer · Pull Request #38361 · pytorch/pytorch

smessmer · 2020-05-12T22:06:15Z

Stack from ghstack:

Unify TensorOptions signatures #39611 Unify TensorOptions signatures
Move TensorOptions ops to c10 #39492 Move TensorOptions ops to c10
Assert that kernels are called with the right signature #38361 Assert that kernels are called with the right signature

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel.

This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly.

Update: It's not in the call path anymore but it's now in the path when you query an OperatorHandle.

supersedes #26482

Differential Revision: D21534052

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) ghstack-source-id: 103966297 Pull Request resolved: #38361

dr-ci · 2020-05-12T23:09:55Z

💊 CI failures summary and remediations

As of commit b04a010 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 135 times.

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

Pull Request resolved: #38361 Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes D17485438 ghstack-source-id: 104070456 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/)

aten/src/ATen/core/boxing/KernelFunction_impl.h

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

Pull Request resolved: #38361 Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes D17485438 ghstack-source-id: 104189472 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/)

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

Pull Request resolved: #38361 Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes D17485438 ghstack-source-id: 104222283 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/)

aten/src/ATen/core/dispatch/OperatorEntry.h

aten/src/ATen/core/dispatch/CppSignature.h

aten/src/ATen/core/dispatch/Dispatcher.h

ezyang · 2020-05-18T15:22:36Z

The test failures might be because in some situations, we have multiple binary compatible versions of the library which have different RTTI. I've noticed this previously in ASAN. Here is a relevant note from the CI:

      # NB: We load libtorch.so with RTLD_GLOBAL for UBSAN, unlike our
      # default behavior.
      #
      # The reason for this is that without RTLD_GLOBAL, if we load multiple
      # libraries that depend on libtorch (as is the case with C++ extensions), we
      # will get multiple copies of libtorch in our address space.  When UBSAN is
      # turned on, it will do a bunch of virtual pointer consistency checks which
      # won't work correctly.  When this happens, you get a violation like:
      #
      #    member call on address XXXXXX which does not point to an object of
      #    type 'std::_Sp_counted_base<__gnu_cxx::_Lock_policy::_S_atomic>'
      #    XXXXXX note: object is of type
      #    'std::_Sp_counted_ptr<torch::nn::LinearImpl*, (__gnu_cxx::_Lock_policy)2>'
      #
      # (NB: the textual types of the objects here are misleading, because
      # they actually line up; it just so happens that there's two copies
      # of the type info floating around in the address space, so they
      # don't pointer compare equal.  See also
      #   https://github.com/google/sanitizers/issues/1175
      #
      # UBSAN is kind of right here: if we relied on RTTI across C++ extension
      # modules they would indeed do the wrong thing;  but in our codebase, we
      # don't use RTTI (because it doesn't work in mobile).  To appease
      # UBSAN, however, it's better if we ensure all the copies agree!
      #
      # By the way, an earlier version of this code attempted to load
      # libtorch_python.so with LD_PRELOAD, which has a similar effect of causing
      # it to be loaded globally.  This isn't really a good idea though, because
      # it depends on a ton of dynamic libraries that most programs aren't gonna
      # have, and it applies to child processes.

One way I know how to fix this problem is if we roll our own RTTI mechanism, manually assembling a type index. But that's a bunch of work. I'm not sure if there's any better option. Resolving the RTLD_GLOBAL problem will be quite challenging in general.

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

Pull Request resolved: #38361 Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes D17485438 ghstack-source-id: 104319639 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/)

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

Pull Request resolved: #38361 Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes D17485438 ghstack-source-id: 104378396 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/)

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

aten/src/ATen/core/dispatch/Dispatcher.h

aten/src/ATen/core/dispatch/OperatorEntry.h

ezyang

yay, much needed!

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

smessmer · 2020-06-18T19:59:15Z

The test failures might be because in some situations, we have multiple binary compatible versions of the library which have different RTTI. I've noticed this previously in ASAN. Here is a relevant note from the CI:

      # NB: We load libtorch.so with RTLD_GLOBAL for UBSAN, unlike our
      # default behavior.
      #
      # The reason for this is that without RTLD_GLOBAL, if we load multiple
      # libraries that depend on libtorch (as is the case with C++ extensions), we
      # will get multiple copies of libtorch in our address space.  When UBSAN is
      # turned on, it will do a bunch of virtual pointer consistency checks which
      # won't work correctly.  When this happens, you get a violation like:
      #
      #    member call on address XXXXXX which does not point to an object of
      #    type 'std::_Sp_counted_base<__gnu_cxx::_Lock_policy::_S_atomic>'
      #    XXXXXX note: object is of type
      #    'std::_Sp_counted_ptr<torch::nn::LinearImpl*, (__gnu_cxx::_Lock_policy)2>'
      #
      # (NB: the textual types of the objects here are misleading, because
      # they actually line up; it just so happens that there's two copies
      # of the type info floating around in the address space, so they
      # don't pointer compare equal.  See also
      #   https://github.com/google/sanitizers/issues/1175
      #
      # UBSAN is kind of right here: if we relied on RTTI across C++ extension
      # modules they would indeed do the wrong thing;  but in our codebase, we
      # don't use RTTI (because it doesn't work in mobile).  To appease
      # UBSAN, however, it's better if we ensure all the copies agree!
      #
      # By the way, an earlier version of this code attempted to load
      # libtorch_python.so with LD_PRELOAD, which has a similar effect of causing
      # it to be loaded globally.  This isn't really a good idea though, because
      # it depends on a ton of dynamic libraries that most programs aren't gonna
      # have, and it applies to child processes.

One way I know how to fix this problem is if we roll our own RTTI mechanism, manually assembling a type index. But that's a bunch of work. I'm not sure if there's any better option. Resolving the RTLD_GLOBAL problem will be quite challenging in general.

CI on the PR is passing now. I'll try landing this and see what happens. If something breaks in the master-only test cases, we might have to revert this.

Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes #26482 Differential Revision: [D21534052](https://our.internmc.facebook.com/intern/diff/D21534052/) [ghstack-poisoned]

facebook-github-bot · 2020-06-18T22:22:27Z

This pull request has been merged in 55cdd31.

Summary: Pull Request resolved: pytorch#38361 Rather than segfaulting, we should show a good error message when in op.call<Return, Args...>(...) the Return type or Args types mismatch the kernel. This adds an assertion comparing two std::type_index to the call path, but that should be fast. Hashing the function signature is also in the call path and not strictly constexpr, but I checked on godbolt that GCC >=5 and Clang >=3.8 optimize it away and make it constexpr, i.e. it's not part of the assembly. supersedes D17485438 ghstack-source-id: 106178820 Test Plan: waitforsandcastle Differential Revision: D21534052 fbshipit-source-id: 6be436a3f20586277a051d764af29e21d5567da0

smessmer mentioned this pull request May 12, 2020

[wip] Error checking for KernelFunction::callUnboxed() #26482

Closed

ezyang reviewed May 14, 2020

View reviewed changes

aten/src/ATen/core/boxing/KernelFunction_impl.h Outdated Show resolved Hide resolved

smessmer requested a review from ezyang May 15, 2020 22:50

ezyang reviewed May 18, 2020

View reviewed changes

aten/src/ATen/core/dispatch/OperatorEntry.h Outdated Show resolved Hide resolved

ezyang reviewed May 18, 2020

View reviewed changes

aten/src/ATen/core/dispatch/CppSignature.h Show resolved Hide resolved

ezyang reviewed May 18, 2020

View reviewed changes

aten/src/ATen/core/dispatch/Dispatcher.h Outdated Show resolved Hide resolved

This was referenced Jun 3, 2020

Show which type was the wrong one when a signature is invalid #39491

Closed

Move TensorOptions ops to c10 #39492

Closed

smessmer requested a review from apaszke as a code owner June 3, 2020 23:13

smessmer mentioned this pull request Jun 5, 2020

Unify TensorOptions signatures #39611

Closed

ezyang reviewed Jun 9, 2020

View reviewed changes

aten/src/ATen/core/dispatch/Dispatcher.h Show resolved Hide resolved

ezyang reviewed Jun 9, 2020

View reviewed changes

aten/src/ATen/core/dispatch/OperatorEntry.h Outdated Show resolved Hide resolved

ezyang approved these changes Jun 9, 2020

View reviewed changes

smessmer mentioned this pull request Jun 10, 2020

TORCH_FN #39823

Closed

smessmer added 9 commits June 11, 2020 13:22

This was referenced Jun 16, 2020

Back out "Revert D21986243: TORCH_FN" #40110

Closed

Add comment with UNBOXEDONLY explanation to codegen #40117

Closed

smessmer added 2 commits June 16, 2020 18:37

facebook-github-bot closed this in 55cdd31 Jun 18, 2020

facebook-github-bot added the merged label Jun 18, 2020

smessmer mentioned this pull request Jun 18, 2020

Assert that kernels are called with the right signature #40251

Closed

facebook-github-bot deleted the gh/smessmer/220/head branch June 22, 2020 14:17

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert that kernels are called with the right signature#38361

Assert that kernels are called with the right signature#38361
smessmer wants to merge 24 commits intogh/smessmer/220/basefrom
gh/smessmer/220/head

smessmer commented May 12, 2020 •

edited

Loading

Uh oh!

dr-ci bot commented May 12, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ezyang commented May 18, 2020

Uh oh!

Uh oh!

Uh oh!

ezyang left a comment

Uh oh!

smessmer commented Jun 18, 2020

Uh oh!

facebook-github-bot commented Jun 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

smessmer commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ezyang commented May 18, 2020

Uh oh!

Uh oh!

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

smessmer commented Jun 18, 2020

Uh oh!

facebook-github-bot commented Jun 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smessmer commented May 12, 2020 •

edited

Loading

dr-ci bot commented May 12, 2020 •

edited

Loading