1556931 - libxul.so: hidden symbol `_ZN2sh16TIntermTraverser8traverseINS_11TIntermNodeEEEvPT_' isn't defined

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P2)

Description

[Mike Hommey [:glandium]]

Under some conditions, the build may fail with:

[task 2019-06-05T02:07:08.768Z] 02:07:08     INFO -  /builds/worker/workspace/build/src/binutils/bin/ld: ../../gfx/angle/targets/translator/RewriteAtomicFunctionExpressions.o: in function `sh::RewriteAtomicFunctionExpressions(sh::TIntermNode*, sh::TSymbolTable*, int)':
[task 2019-06-05T02:07:08.768Z] 02:07:08     INFO -  /builds/worker/workspace/build/src/gfx/angle/checkout/src/compiler/translator/tree_ops/RewriteAtomicFunctionExpressions.cpp:179: undefined reference to `void sh::TIntermTraverser::traverse<sh::TIntermNode>(sh::TIntermNode*)'
[task 2019-06-05T02:07:08.768Z] 02:07:08     INFO -  /builds/worker/workspace/build/src/binutils/bin/ld: libxul.so: hidden symbol `_ZN2sh16TIntermTraverser8traverseINS_11TIntermNodeEEEvPT_' isn't defined

The reason this happens is that the code actually relies on the compiler not inlining that function, which, under some conditions, it does.

The fix is to explicitly instantiate a template, like the following:

diff --git a/gfx/angle/checkout/src/compiler/translator/tree_util/IntermTraverse.cpp b/gfx/angle/checkout/src/compiler/translator/tree_util/IntermTraverse.cpp
index c38baa12bffa8..2858a7bb4d319 100644
--- a/gfx/angle/checkout/src/compiler/translator/tree_util/IntermTraverse.cpp
+++ b/gfx/angle/checkout/src/compiler/translator/tree_util/IntermTraverse.cpp
@@ -45,16 +45,20 @@ void TIntermTraverser::traverse(T *node)
             ++childIndex;
         }
 
         if (visit && postVisit)
             node->visit(PostVisit, this);
     }
 }
 
+// Instantiate template for RewriteAtomicFunctionExpressions.
+template
+void TIntermTraverser::traverse(TIntermNode *);
+
 void TIntermNode::traverse(TIntermTraverser *it)
 {
     it->traverse(this);
 }
 
 void TIntermSymbol::traverse(TIntermTraverser *it)
 {
     TIntermTraverser::ScopedNodeInTraversalPath addToPath(it, this);

I would send this upstream if they didn't require a CLA.

Jeff, Lee, can one of you look into this?

Comment 1

[Mike Hommey [:glandium]]

I wonder if we could pro-actively find these with some static analysis, rather than rely on luck.

Comment 2

[Andi [:andi]]

After talking with Mike on irc we've concluded that there is no solution using our current clang based static-analysis to intercept these kind of things, we will be able detect these issues once we will do CTU analysis.

Comment 3

[Lee Salzman [:lsalzman]]

Jeff should be able to look into fixing this.

Comment 4

[Kelsey Gilbert [:jgilbert]]

I'm not entirely following how it can "only sometimes (mis-)compile"?

Comment 5

[Kelsey Gilbert [:jgilbert]]

TBH it feels like an optimizer bug if it decides, looking only at one TU, to inline a non-hidden function. It's unclear to me why it's allowed to not export that function from the TU.

Comment 6

[Mike Hommey [:glandium]]

Depends on optimization, whether the monomorphized function is inlined or not. The fix in comment 0 works, it "just" needs to be upstreamed.

Comment 7

[Mike Hommey [:glandium]]

TBH it feels like an optimizer bug

It's not. That's how templates work in C++.

Comment 8

[Kelsey Gilbert [:jgilbert]]

I want to know why, before I can explain that we need to upstream it. Otherwise you'll have to sign the CLA and convince them yourself!

Comment 9

[Kelsey Gilbert [:jgilbert]]

Ok, I get it. I think I see an approach to make this more obvious.

Comment 10

[Mike Hommey [:glandium]]

Even if it is a compiler bug, the fact is the failure does happen. Working around it is necessary. Now, as for why: the template instantiation is inlined in IntermTraverse.cpp, so in practice, it's the same as if the template was never instantiated. When it's used in RewriteAtomicFunctionExpressions.cpp, the template can't be instantiated because it's not defined in the header, it's only declared there. An alternative to the explicit instantiation is to move the definitions into the header. That's probably not desirable.

Comment 11

[Kelsey Gilbert [:jgilbert]]

I think we might also be able to require this in IntermTraverse.h:

// Needed by RewriteAtomicFunctionExpressions:
template void TIntermTraverser::traverse<TIntermNode>(TIntermNode*);

That should commit the compiler to exporting that template instantiation, and then we can warn/error when calling TUs link to a non-explicit instantiation?

Comment 12

[Kelsey Gilbert [:jgilbert]]

Does that sound like a path towards determinism?

Comment 13

[Mike Hommey [:glandium]]

Doing that requires the template definition, so compiling RewriteAtomicFunctionExpressions.cpp would fail. The only place for explicit instantiation is a .cpp, not a header.

Comment 14

[Mike Hommey [:glandium]]

At least, when the definition is in the .cpp.

Comment 15

[Kelsey Gilbert [:jgilbert]]

It doesn't seem to! I tested compiling with that, and it definitely compiles.

Comment 16

[Mike Hommey [:glandium]]

I don't see how. This would be equivalent of:

template<typename T> void foo(T*);
template void foo<int>(int*);

And the above fails to build with

foo.cc:2:15: error: explicit instantiation of undefined function template 'foo'

Comment 17

[Mike Hommey [:glandium]]

Try says adding the template instantiation in the header fails building e.g. ASTMetadataHLSL.o with

/builds/worker/workspace/build/src/gfx/angle/checkout/src/compiler/translator/tree_util/IntermTraverse.h:282:33: error: explicit instantiation of undefined function template 'traverse'

Comment 18

[Kelsey Gilbert [:jgilbert]]

Thanks, lost track of this.

Comment 19

[Kelsey Gilbert [:jgilbert]]

https://bugs.chromium.org/p/angleproject/issues/detail?id=3752
https://chromium-review.googlesource.com/c/angle/angle/+/1721954

Comment 20

[Dan Horák]

Hi, what's the status of this issue? Because I have got same problem in bug 1654490 (with a different fix).

Comment 21

[Olivier Tilloy]

Attachment: Bug 1556931 - cherry-pick upstream angle fix for a hidden symbol when the compiler inlines a template function.

Comment 23

[Mike Kaply [:mkaply]]

We're upstreaming Angle in Firefox 87, so we don't think it's worth taking this right now. Folks will need to continue to apply the patch.

Comment 24

[Dan Horák]

works for me