LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 11250 - no code emitted for virtual inline function inherited indirectly from class template
Summary: no code emitted for virtual inline function inherited indirectly from class t...
Status: RESOLVED INVALID
Alias: None
Product: clang
Classification: Unclassified
Component: -New Bugs (show other bugs)
Version: trunk
Hardware: PC Linux
: P normal
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks: 12255
  Show dependency tree
 
Reported: 2011-10-28 05:19 PDT by Stephan Bergmann
Modified: 2012-03-12 13:34 PDT (History)
4 users (show)

See Also:
Fixed By Commit(s):


Attachments
test case (521 bytes, application/x-shellscript)
2011-10-28 05:19 PDT, Stephan Bergmann
Details
Alternative testcase (518 bytes, application/x-shellscript)
2012-03-10 08:47 PST, Luboš Luňák
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Bergmann 2011-10-28 05:19:10 PDT
Created attachment 7544 [details]
test case

The attached run.sh fails for me (on Fedora 15 x86_64) with Clang built from today's trunk with the following output:

++ cat
++ cat
++ clang++ --version
clang version 3.1 (trunk 143187)
Target: x86_64-unknown-linux-gnu
Thread model: posix
++ clang++ -shared -fPIC -fvisibility=hidden lib1.cc -o lib1.so
++ clang++ -shared -fPIC lib2.cc -L. -l1 -Wl,-z,defs -o lib2.so
/tmp/lib2-I0DZ8n.o:(.data.rel.ro+0x20): undefined reference to `S1<int>::f()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

GCC on the other hand works fine, emitting f() weakly in lib2.o.
Comment 1 Stephan Bergmann 2011-10-28 09:39:38 PDT

*** This bug has been marked as a duplicate of bug 10113 ***
Comment 2 Eli Friedman 2011-10-28 18:07:44 PDT
Reopening; I'm convinced this is a different issue.
Comment 3 Eli Friedman 2011-10-28 18:08:38 PDT
A straightforward test for whether a compiler does the "right" thing here is
whether the following compiles (it gives an error with gcc):

template<typename T> struct __attribute((visibility("hidden"))) S1 {
  virtual ~S1() {}
  virtual void f() { this->DOESNOTEXIST; }
};
struct S2: S1<int> { virtual ~S2(); };
struct S3: S2 { virtual ~S3(); };
S3::~S3() {}

The relevant code in clang is Sema::MarkVirtualMembersReferenced.

Note that this version gives the warning "warning: ‘S2’ declared with greater
visibility than its base ‘S1<int>’" with gcc; I think it's a gcc bug that this
warning doesn't show up when using -fvisibility=hidden.   In the general case,
a non-hidden class cannot inherit from a hidden class; code including such
constructs is suspicious at best.
Comment 4 Luboš Luňák 2012-03-10 08:47:09 PST
Created attachment 8171 [details]
Alternative testcase

This modified testcase replaces the explicit visibility markup in the source files by usage of -fvisibility-inlines-hidden.

In the original testcase, one could argue that the problem is in the source files and not in clang. In lib2.cc, clang is apparently smart enough to see that it is not necessary to instantiate S1< int >, because it will be done in the same place where S2::~S2() is implemented (since S2's vtable is keyed to it, as the first virtual method in S2, and S2's vtable will require vtable of S1< int >, which will also instantiate S1< int >::f()). Therefore it could be argued that it is a developer error to mark S1 as non-exported (especially since it's done so only in lib1.cc and not in lib2.cc ).

However, this new testcase compiles and links fine as it is, as long as -fvisibility-inlines-hidden is not used, otherwise the same problem occurs. Now again lib1.so contains a non-exported instance of S1< int >::f(), because of the switch.

Presumably when compiling with -fvisibility-inlines-hidden clang should create a non-exported instance of S1< int >::f() even in lib2.so , otherwise -fvisibility-inlines-hidden is broken or next to useless.
Comment 5 Stephan Bergmann 2012-03-12 03:15:09 PDT
Re comment 3:

1  It is unclear to me what "this->DOESNOTEXIST;" shall show?  Of course, a compiler shall reject that code, irrespective of any visibility annotations.  What am I missing?

2  "In the general case, a non-hidden class cannot inherit from a hidden class":  I wonder whether this approach is practical in cases where the base is an (all-inline) template.  It looks unfortunate to me that one should be required to make the base non-hidden, and I do not see why keeping it hidden would necessarily be suspicious?
Comment 6 Eli Friedman 2012-03-12 04:19:26 PDT
(In reply to comment #5)
> Re comment 3:
> 
> 1  It is unclear to me what "this->DOESNOTEXIST;" shall show?  Of course, a
> compiler shall reject that code, irrespective of any visibility annotations. 
> What am I missing?

It only gets rejected if the function is instantiated.  clang doesn't instantiate the function, so there's no error.

> 2  "In the general case, a non-hidden class cannot inherit from a hidden
> class":  I wonder whether this approach is practical in cases where the base is
> an (all-inline) template.  It looks unfortunate to me that one should be
> required to make the base non-hidden, and I do not see why keeping it hidden
> would necessarily be suspicious?

It's generally wrong because you end up breaking ODR. Given that there is a definition of both the base and derived classes in two different libraries, the two definitions of the base class are unrelated because they are hidden.  Therefore, the definitions of the derived class break ODR because they refer to different types.
Comment 7 Stephan Bergmann 2012-03-12 11:43:10 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Re comment 3:
> > 
> > 1  It is unclear to me what "this->DOESNOTEXIST;" shall show?  Of course, a
> > compiler shall reject that code, irrespective of any visibility annotations. 
> > What am I missing?
> 
> It only gets rejected if the function is instantiated.  clang doesn't
> instantiate the function, so there's no error.

But the behavior of GCC (emitting an error) is not wrong either, right?  The standard leaves it unspecified whether virtual function members are implicitly instantiated when the class template is implicitly instantiated.

> > 2  "In the general case, a non-hidden class cannot inherit from a hidden
> > class":  I wonder whether this approach is practical in cases where the base is
> > an (all-inline) template.  It looks unfortunate to me that one should be
> > required to make the base non-hidden, and I do not see why keeping it hidden
> > would necessarily be suspicious?
> 
> It's generally wrong because you end up breaking ODR. Given that there is a
> definition of both the base and derived classes in two different libraries, the
> two definitions of the base class are unrelated because they are hidden. 
> Therefore, the definitions of the derived class break ODR because they refer to
> different types.

But why would hidden-ness of the base necessarily imply two unrelated classes (and thus ODR-violation of the derived class)?  An alternative interpretation of how (non-standard) visibility interacts with (standard) ODR could treat the two hidden occurrences of the base as the same type.  (An interpretation that, it looks to me, is followed by GCC -- even if it indeed issues a warning under certain conditions.)
Comment 8 Eli Friedman 2012-03-12 13:07:43 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Re comment 3:
> > > 
> > > 1  It is unclear to me what "this->DOESNOTEXIST;" shall show?  Of course, a
> > > compiler shall reject that code, irrespective of any visibility annotations. 
> > > What am I missing?
> > 
> > It only gets rejected if the function is instantiated.  clang doesn't
> > instantiate the function, so there's no error.
> 
> But the behavior of GCC (emitting an error) is not wrong either, right?  The
> standard leaves it unspecified whether virtual function members are implicitly
> instantiated when the class template is implicitly instantiated.

Yes.  But both clang and gcc try to avoid instantiating virtual functions if possible.

> > > 2  "In the general case, a non-hidden class cannot inherit from a hidden
> > > class":  I wonder whether this approach is practical in cases where the base is
> > > an (all-inline) template.  It looks unfortunate to me that one should be
> > > required to make the base non-hidden, and I do not see why keeping it hidden
> > > would necessarily be suspicious?
> > 
> > It's generally wrong because you end up breaking ODR. Given that there is a
> > definition of both the base and derived classes in two different libraries, the
> > two definitions of the base class are unrelated because they are hidden. 
> > Therefore, the definitions of the derived class break ODR because they refer to
> > different types.
> 
> But why would hidden-ness of the base necessarily imply two unrelated classes
> (and thus ODR-violation of the derived class)?  An alternative interpretation
> of how (non-standard) visibility interacts with (standard) ODR could treat the
> two hidden occurrences of the base as the same type.  (An interpretation that,
> it looks to me, is followed by GCC -- even if it indeed issues a warning under
> certain conditions.)

Any sort of model where two hidden visibility classes in different libraries can sometimes be treated as identical for odr is rather complicated.  You can't always treat two classes with the same name as identical because then you end up in the exact situation hidden visibility is supposed to avoid: the names can conflict.  Also, some classes depend on ODR to work as expected (for example, using static data, or comparing member function pointers).
Comment 9 Stephan Bergmann 2012-03-12 13:09:16 PDT
(In reply to comment #7)
> But why would hidden-ness of the base necessarily imply two unrelated classes
> (and thus ODR-violation of the derived class)?  An alternative interpretation
> of how (non-standard) visibility interacts with (standard) ODR could treat the
> two hidden occurrences of the base as the same type.  (An interpretation that,
> it looks to me, is followed by GCC -- even if it indeed issues a warning under
> certain conditions.)

I just found out that I am probably wrong with that assumption (cf. <http://lists.freedesktop.org/archives/libreoffice/2012-March/028063.html> "Exporting templates on Windows"):

"I see no fundamental reason why [a hidden template with static data member] should not work.  I would argue that the compiler could still export that symbol with some form of vague linkage, as it does if the template is not hidden.  But I just see that GCC does not do that, and even somewhat specifies that it does not do that ('type visibility is applied to vague linkage entities associated with the class', <http://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Type-Attributes.html#Type-Attributes> in combination with 'Most everything in this section [on vague linkage] also applies to template instantiations', <http://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Vague-Linkage.html#Vague-Linkage>).

"Looks like a misunderstanding whether hidden visibility is intended for 'remove unnecessary entries from dynamic symbol tables (but keeping certain symbols exported to not violate certain standard requirements)' or 'allow violations of ODR by hiding classes completely from dynamic symbol tables.'  I had naively assumed the former, while compiler writers apparently use the latter interpretation."
Comment 10 Stephan Bergmann 2012-03-12 13:28:37 PDT
So I close this as INVALID.  Luboš told me he will move the -fvisibility-inlines-hidden issue from comment 4 to a new bug.
Comment 11 Luboš Luňák 2012-03-12 13:34:07 PDT
Bug #12255 created for the related issue.