RE: Missing definition of a template virtual function

[Srivastava, Sunil via cfe-commits]

Hi Richard,

Thanks for you analysis. I have some follow up questions though:

First, either of these two suggestions below should fix the bug, though it may 
be better to do both. Right ?

Then, approach (2) has a cost. It will prevent devirtualizations of calls which 
could get defined elsewhere.

While my example of https://llvm.org/bugs/show_bug.cgi?id=27895#c4   is good 
for demonstrating the bug, it is an unusual case. The usual case is that the 
template will be defined somewhere. So by suppressing devirtualization in this 
case we will be penalizing the usual cases to fix a bug in unusual cases.

The approach (1) is doable of course. It involves passing some kind of 
isFullObject flag few levels down in SemaOverload.cpp, and then prevent reset 
of OdrUse flag if isFullObject is true.

However I want to understand better what is going on here. Specifically, why 
virtual operators are treated different from virtual functions. In my example, 
if you replace the operator[] with a plain function doing the same thing, the 
function definition gets generated.

In your bugzilla comment 5, you perhaps alluded to this point:


> we model operator[] as weak_odr unnamed_addr, which would allow us to

> discard the symbol in the other translation unit if we can merge the

> function with something else, even though its address is exposed in

> the vtable

But what can the compiler do with user written operator[] that it can not do 
with a plain function ?

Beside, this bug is not just with operator[]. It is with every member operator.

Thanks
Sunil Srivastava
Sony Interactive Entertainment

Yes, this is a bug. We emit a direct call to TmplWithArray::operator[] 
so we're required to also emit a definition of it. I think the code that 
r227073 removed was really masking this bug rather than fixing it. It seems 
like there are two issues here:

 1) Sema should mark a virtual function as used if it sees an 
obviously-devirtualizable call to the function (where the base object has a 
known dynamic type)

 2) Clang CodeGen's devirtualization of obviously-devirtualizable calls should 
not devirtualize calls to linkonce_odr functions that it can't emit



--
template < typename T, int N = 0 > class TmplWithArray {
public:
  virtual T& operator [] (int idx);
  T ar[N+1];
};
template  T& TmplWithArray::operator[](int idx) {
  return ar[idx];
}
class Wrapper {
  TmplWithArray data;
  bool indexIt(int a);
};
bool Wrapper::indexIt(int a)
{
   return data[a];
}
int main(){}
--

Starting from  r227073 it does not link (at any optimization level). The code
has a direct call to the operator[] function, but that function definition is
not generated.

Versions prior to r227073,
-  at –O0 or –O1, generate the operator[] function and the direct call, and
-  at –O2 do not generate the function, but inline it
Either way they link fine.

In this example the key-function for the generation of the vtable is the
operator[] function itself.

So the compiler can either generate both the vtable and the operator[]
function, or not generate either; they are both consistent states.

The call in data[a] is to a virtual function, and if the compiler left it as a
virtual call, it will link. There will be no ctor, no vtable, and no operator[]
function. Wrapper::indexIt will be dead code, but it will link.

But the compiler does devirtualization of the call and generates a direct call,
yet, beginning with r227073, it does not generate the operator[] function.
Hence the link failure.

Another interesting tidbit is that if the operator[] is replaced by a plain
function doing the same thing, along with the corresponding change in the
usage, something like:
virtual T& getElt(int idx);
then the behavior is the same as pre r227073. Either getElt is defined or it
gets inlined.

BTW, this is not just an idle curiosity. This example was trimmed down from
a user bug report having a link failure.

Sunil Srivastava
Sony Interactive Entertainment

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Missing definition of a template virtual function

[Richard Smith via cfe-commits]

On Sun, Jun 5, 2016 at 10:27 PM, Srivastava, Sunil <
sunil.srivast...@sony.com> wrote:

> Hi Richard,
>
>
>
> Thanks for you analysis. I have some follow up questions though:
>
>
> First, either of these two suggestions below should fix the bug, though it
> may be better to do both. Right ?
>

Either should fix the immediate issue, but if we don't do (2) then the bug
will likely just be more subtle.


> Then, approach (2) has a cost. It will prevent devirtualizations of calls
> which could get defined elsewhere.
>

It will never prevent a correct devirtualization; it is never correct for
us to emit a direct reference to a function that we would emit as
linkonce_odr but that we can't emit. (We can do so if we know there's a
weak_odr definition somewhere else that we're allowed to reference, but in
that case we can and should emit the function locally as
available_externally instead.)

While my example of https://llvm.org/bugs/show_bug.cgi?id=27895#c4   is
> good for demonstrating the bug, it is an unusual case. The usual case is
> that the template will be defined somewhere. So by suppressing
> devirtualization in this case we will be penalizing the usual cases to fix
> a bug in unusual cases.
>
>
>
> The approach (1) is doable of course. It involves passing some kind of
> isFullObject flag few levels down in SemaOverload.cpp, and then prevent
> reset of OdrUse flag if isFullObject is true.
>
>
>
> However I want to understand better what is going on here. Specifically,
> why virtual operators are treated different from virtual functions.
>

I would expect the difference is operator syntax versus function call
syntax -- these go down quite different codepaths in Sema and it's quite
likely that they mark the function as used in subtly different ways. It
would make sense to unify the behavior here.


> In my example, if you replace the operator[] with a plain function doing
> the same thing, the function definition gets generated.
>
>
>
> In your bugzilla comment 5, you perhaps alluded to this point:
>
>
>
> > we model operator[] as weak_odr unnamed_addr, which would allow us to
>
> > discard the symbol in the other translation unit if we can merge the
>
> > function with something else, even though its address is exposed in
>
> > the vtable
>
>
>
> But what can the compiler do with user written operator[] that it can not
> do with a plain function ?
>
>
>
> Beside, this bug is not just with operator[]. It is with every member
> operator.
>
>
>
> Thanks
>
> Sunil Srivastava
>
> Sony Interactive Entertainment
>
>
>
> Yes, this is a bug. We emit a direct call to
> TmplWithArray::operator[] so we're required to also emit a
> definition of it. I think the code that r227073 removed was really masking
> this bug rather than fixing it. It seems like there are two issues here:
>
>
>
>  1) Sema should mark a virtual function as used if it sees an
> obviously-devirtualizable call to the function (where the base object has a
> known dynamic type)
>
>
>
>  2) Clang CodeGen's devirtualization of obviously-devirtualizable calls
> should not devirtualize calls to linkonce_odr functions that it can't emit
>
>
>
>
>
>
>
> --
>
> template < typename T, int N = 0 > class TmplWithArray {
>
> public:
>
>   virtual T& operator [] (int idx);
>
>   T ar[N+1];
>
> };
>
> template  T& TmplWithArray::operator[](int idx) {
>
>   return ar[idx];
>
> }
>
> class Wrapper {
>
>   TmplWithArray data;
>
>   bool indexIt(int a);
>
> };
>
> bool Wrapper::indexIt(int a)
>
> {
>
>return data[a];
>
> }
>
> int main(){}
>
> --
>
>
>
> Starting from  r227073 it does not link (at any optimization level). The
> code
>
> has a direct call to the operator[] function, but that function definition
> is
>
> not generated.
>
>
>
> Versions prior to r227073,
>
> -  at –O0 or –O1, generate the operator[] function and the direct
> call, and
>
> -  at –O2 do not generate the function, but inline it
>
> Either way they link fine.
>
>
>
> In this example the key-function for the generation of the vtable is the
>
> operator[] function itself.
>
>
>
> So the compiler can either generate both the vtable and the operator[]
>
> function, or not generate either; they are both consistent states.
>
>
>
> The call in data[a] is to a virtual function, and if the compiler left it
> as a
>
> virtual call, it will link. There will be no ctor, no vtable, and no
> operator[]
>
> function. Wrapper::indexIt will be dead code, but it will link.
>
>
>
> But the compiler does devirtualization of the call and generates a direct
> call,
>
> yet, beginning with r227073, it does not generate the operator[] function.
>
> Hence the link failure.
>
>
>
> Another interesting tidbit is that if the operator[] is replaced by a plain
>
> function doing the same thing, along with the corresponding change in the
>
> usage, something like:
>
> virtual T& getElt(int 

Re: Missing definition of a template virtual function

[Richard Smith via cfe-commits]

On Fri, May 27, 2016 at 1:03 PM, Srivastava, Sunil via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Hi,
>
>
>
> I want to discuss the issue in PR27895,
>
>https://llvm.org/bugs/show_bug.cgi?id=27895
>
>
>
> But I will give a slightly different example for this discussion.
>
>
>
> The issue can be demonstrated by this standalone code. Should this example
> link
>
> successfully?
>

Yes, this is a bug. We emit a direct call to
TmplWithArray::operator[] so we're required to also emit a
definition of it. I think the code that r227073 removed was really masking
this bug rather than fixing it. It seems like there are two issues here:

 1) Sema should mark a virtual function as used if it sees an
obviously-devirtualizable call to the function (where the base object has a
known dynamic type)

 2) Clang CodeGen's devirtualization of obviously-devirtualizable calls
should not devirtualize calls to linkonce_odr functions that it can't emit

--
>
> template < typename T, int N = 0 > class TmplWithArray {
>
> public:
>
>   virtual T& operator [] (int idx);
>
>   T ar[N+1];
>
> };
>
> template  T& TmplWithArray::operator[](int idx) {
>
>   return ar[idx];
>
> }
>
> class Wrapper {
>
>   TmplWithArray data;
>
>   bool indexIt(int a);
>
> };
>
> bool Wrapper::indexIt(int a)
>
> {
>
>return data[a];
>
> }
>
> int main(){}
>
> --
>
>
>
> Starting from  r227073 it does not link (at any optimization level). The
> code
>
> has a direct call to the operator[] function, but that function definition
> is
>
> not generated.
>
>
>
> Versions prior to r227073,
>
> -  at –O0 or –O1, generate the operator[] function and the direct
> call, and
>
> -  at –O2 do not generate the function, but inline it
>
> Either way they link fine.
>
>
>
> In this example the key-function for the generation of the vtable is the
>
> operator[] function itself.
>
>
>
> So the compiler can either generate both the vtable and the operator[]
>
> function, or not generate either; they are both consistent states.
>
>
>
> The call in data[a] is to a virtual function, and if the compiler left it
> as a
>
> virtual call, it will link. There will be no ctor, no vtable, and no
> operator[]
>
> function. Wrapper::indexIt will be dead code, but it will link.
>
>
>
> But the compiler does devirtualization of the call and generates a direct
> call,
>
> yet, beginning with r227073, it does not generate the operator[] function.
>
> Hence the link failure.
>
>
>
> Another interesting tidbit is that if the operator[] is replaced by a plain
>
> function doing the same thing, along with the corresponding change in the
>
> usage, something like:
>
> virtual T& getElt(int idx);
>
> then the behavior is the same as pre r227073. Either getElt is defined or
> it
>
> gets inlined.
>
>
>
> BTW, this is not just an idle curiosity. This example was trimmed down from
>
> a user bug report having a link failure.
>
>
>
> Sunil Srivastava
>
> Sony Interactive Entertainment
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Missing definition of a template virtual function

[Srivastava, Sunil via cfe-commits]

Hi,

I want to discuss the issue in PR27895,
   https://llvm.org/bugs/show_bug.cgi?id=27895

But I will give a slightly different example for this discussion.

The issue can be demonstrated by this standalone code. Should this example link
successfully?
--
template < typename T, int N = 0 > class TmplWithArray {
public:
  virtual T& operator [] (int idx);
  T ar[N+1];
};
template  T& TmplWithArray::operator[](int idx) {
  return ar[idx];
}
class Wrapper {
  TmplWithArray data;
  bool indexIt(int a);
};
bool Wrapper::indexIt(int a)
{
   return data[a];
}
int main(){}
--

Starting from  r227073 it does not link (at any optimization level). The code
has a direct call to the operator[] function, but that function definition is
not generated.

Versions prior to r227073,
-  at -O0 or -O1, generate the operator[] function and the direct call, and
-  at -O2 do not generate the function, but inline it
Either way they link fine.

In this example the key-function for the generation of the vtable is the
operator[] function itself.

So the compiler can either generate both the vtable and the operator[]
function, or not generate either; they are both consistent states.

The call in data[a] is to a virtual function, and if the compiler left it as a
virtual call, it will link. There will be no ctor, no vtable, and no operator[]
function. Wrapper::indexIt will be dead code, but it will link.

But the compiler does devirtualization of the call and generates a direct call,
yet, beginning with r227073, it does not generate the operator[] function.
Hence the link failure.

Another interesting tidbit is that if the operator[] is replaced by a plain
function doing the same thing, along with the corresponding change in the
usage, something like:
virtual T& getElt(int idx);
then the behavior is the same as pre r227073. Either getElt is defined or it
gets inlined.

BTW, this is not just an idle curiosity. This example was trimmed down from
a user bug report having a link failure.

Sunil Srivastava
Sony Interactive Entertainment
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits