ATen icon indicating copy to clipboard operation
ATen copied to clipboard

Put Tensor inline method definitions in Tensor.h

Open ezyang opened this issue 6 years ago • 0 comments

Consider the following C++ transcript:

MacBook-Pro-97:cpp-inline-method ezyang$ cat A.h
void f();
MacBook-Pro-97:cpp-inline-method ezyang$ cat B.h
#include "A.h"
#include <iostream>
inline void f() { std::cerr << "hello world"; }
MacBook-Pro-97:cpp-inline-method ezyang$ cat C.cpp
#include "A.h"
int main() {
  f();
}
MacBook-Pro-97:cpp-inline-method ezyang$ g++ C.cpp 
Undefined symbols for architecture x86_64:
  "f()", referenced from:
      _main in C-4671ba.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

We define a forward declaration of a function in A.h, and then define an inline definition of it in B.h. When we build C.cpp and accidentally include A.h instead of B.h, the symbol is not found. Obviously this cannot work.

However, we have constructed precisely this situation in ATen:

  • ATen/Tensor.h contains forward declarations of tensor methods
  • ATen/TensorMethods.h contains inline definitions of these tensor methods
  • If you include ATen/Tensor.h without somehow including ATen/TensorMethods.h, then you will get an undefined symbol error, because the inline definition does not get inlined (there's no way we know about it!)

The party line is that, external clients should always include ATen/ATen.h. But internal clients can't do this, since it will cause header cycles. We should rearrange the headers so that this situation cannot happen, and so you don't get inexplicable symbol missing problems.

EDIT: It occurs to me that there's nothing stopping cpp files in ATen from including ATen/ATen.h, and indeed this seems sufficient to solve problems of this kind. But it is still very easy to get wrong. Consider the following idiom, for a file named ATen/Foo.h:

#include "ATen/Foo.h"

// Your code here

If this compiled, you might be tempted to conclude that all is well and you don't need to add any more header definitions. But you'd be wrong, if ATen/Foo.h only included Tensor.h and not TensorMethods.h. You would not discover this until link time.

ezyang avatar Nov 18 '17 02:11 ezyang