[Bug c++/35262] New: [4.4 Regression]: FAIL: abi_check

[Bug c++/35262] New: [4.4 Regression]: FAIL: abi_check

[hjl dot tools at gmail dot com]

With gcc 4.4 revision 132478 on Linux/Intel64, I got

FAIL: abi_check

in libstdc++. Revision 132408 is OK.


-- 
           Summary: [4.4 Regression]: FAIL: abi_check
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: hjl dot tools at gmail dot com
GCC target triplet: x86_64-unknown-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hjl dot tools at gmail dot com]

------- Comment #1 from hjl dot tools at gmail dot com  2008-02-20 15:50 -------
Revision 132439 is bad:

http://gcc.gnu.org/ml/gcc-testresults/2008-02/msg01322.html

Revision 132433 is OK:

http://gcc.gnu.org/ml/gcc-testresults/2008-02/msg01318.html

The only change in 4.4 between those 2 revisions is revision 132439:

http://gcc.gnu.org/ml/gcc-cvs/2008-02/msg00454.html

Jan, is this related to your patch?


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu dot
                   |                            |org
 GCC target triplet|x86_64-unknown-linux-gnu    |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pinskia at gcc dot gnu dot org]

------- Comment #2 from pinskia at gcc dot gnu dot org  2008-02-20 17:50 -------
(In reply to comment #1)
> Jan, is this related to your patch?

And if it is, then there is another bug somewhere else anyways as it could only
change inlining decisions.

-- Pinski


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at ucw dot cz]

------- Comment #3 from hubicka at ucw dot cz  2008-02-20 21:05 -------
Subject: Re:  [4.4 Regression]: FAIL: abi_check

> (In reply to comment #1)
> > Jan, is this related to your patch?
> 
> And if it is, then there is another bug somewhere else anyways as it could only
> change inlining decisions.

It might be the negative inlining cost assert.  I will look into that.

Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #4 from pcarlini at suse dot de  2008-02-20 21:13 -------
I'm not sure there is a substantive bug here, in that the problem can be easily
fixed by simply tweaking gnu.ver to explicitely hide
ZSt13__check_facetISt7codecvtIcc11__mbstate_tEERKT_PS4_ and the wchar_t
counterpart: certainly would not be the first time exports must be tweaked
because of a changing inlining decision. My concern, however, is whether it's
ok not to inline such a tiny function (fyi, the function is defined in
basic_ios.h all the uses are in fstream.tcc). First blush, I don't think it's
ok...


-- 

pcarlini at suse dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pcarlini at suse dot de


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at ucw dot cz]

------- Comment #5 from hubicka at ucw dot cz  2008-02-20 23:39 -------
Subject: Re:  [4.4 Regression]: FAIL: abi_check

OK,
if it really is just inlining decision difference then we are fine.
I guess we can either update symbol list or mark always_inline
> because of a changing inlining decision. My concern, however, is whether it's
> ok not to inline such a tiny function (fyi, the function is defined in
> basic_ios.h all the uses are in fstream.tcc). First blush, I don't think it's
> ok...

I can look into the reason why it is not getting inlined. It would help
to have preprocessed testcase as I am travelling now  :)

Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #6 from pcarlini at suse dot de  2008-02-21 00:07 -------
(In reply to comment #5)
> Subject: Re:  [4.4 Regression]: FAIL: abi_check
> 
> OK,
> if it really is just inlining decision difference then we are fine.
> I guess we can either update symbol list or mark always_inline

Yes, from a robustness of the set of exported symbols point of view eventually
we should anyway specify in the linker script to hide such symbols. However...

> I can look into the reason why it is not getting inlined. It would help
> to have preprocessed testcase as I am travelling now  :)

... many thanks! Because I think 4.3.0 is right here, I think that small
function should be indeed inlined. I'm going to add a trivial preprocessed
file, which just instantiates std::basic_filebuf<char>: at -O2 the object
contains the __check_facet<codecvt> symbol, at variance with 4.3. Many thanks
again for looking into this.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #7 from pcarlini at suse dot de  2008-02-21 00:08 -------
Created an attachment (id=15189)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=15189&action=view)
Pre-processed instantiation


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #8 from pcarlini at suse dot de  2008-02-24 13:45 -------
*** Bug 35351 has been marked as a duplicate of this bug. ***


-- 

pcarlini at suse dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jrp at dial dot pipex dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #9 from pcarlini at suse dot de  2008-03-02 17:35 -------
Confirmed, of course. Honza, any news on the inlining issue?


-- 

pcarlini at suse dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2008-03-02 17:35:18
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at ucw dot cz]

------- Comment #10 from hubicka at ucw dot cz  2008-03-03 00:50 -------
Subject: Re:  [4.4 Regression]: FAIL: abi_check

> Confirmed, of course. Honza, any news on the inlining issue?
Sorry,
I looked into it, got confused and then distracted by other problem and
forgot to return back.

At second look, the function is estimated to make code to grow slightly
after being inlined.  The functions is still getting inlined by default,
however there are code paths are marked by __builtin_expect as unlikely.
The call sites on these paths are considered cold and thus function is
not inlined there to optimize for size.  This seems very sane behaviour
at first sight.

However the catch is that the function is bit bigger than call sequence
but still after being fully inlined, the overall code size is estimated
to shrink because offline body is eliminated.  So it is definitly better
to inline and have more options for optimizing.  Quite corner case but
easy enough to handle.

I am testing patch to take this fact into account and lets see if it
solves to failure too.  It gets function in question inlined, but makes
other not inlined this time ;)

Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at gcc dot gnu dot org]

------- Comment #11 from hubicka at gcc dot gnu dot org  2008-03-03 16:21 -------
Subject: Bug 35262

Author: hubicka
Date: Mon Mar  3 16:20:31 2008
New Revision: 132838

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132838
Log:
        PR c++/35262
        * ipa-inline.c (cgraph_decide_inlining_of_small_function): Be more
        aggressive on inlining cold calls.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-inline.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at gcc dot gnu dot org]

------- Comment #12 from hubicka at gcc dot gnu dot org  2008-03-03 16:23 -------
Fixed.


-- 

hubicka at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #13 from pcarlini at suse dot de  2008-03-03 19:04 -------
Honza, I'm sorry, can you please double-check the fix? On my x86_64-linux
machines I'm not seeing any progress :(


-- 

pcarlini at suse dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at ucw dot cz]

------- Comment #14 from hubicka at ucw dot cz  2008-03-03 23:46 -------
Subject: Re:  [4.4 Regression]: FAIL: abi_check

> Honza, I'm sorry, can you please double-check the fix? On my x86_64-linux
> machines I'm not seeing any progress :(
Hi,
this is what I get from our thester:

Differences:
Tests that now work, but didn't before:
abi_check

so it ought to make differnece for i686-linux.

It is quite possible that things differ on 64bit hosts, we are staying
on quite fragile ground here because in the current cost metric the
benefits of inlining are very close to costs. Given the nature that
function call of the wrapped function is a bit chepaer than call of the
wrapper is quite correct.

The decision on whether function should be inlined or not depends on
many things, like overall size, ABI details etc.  I see it is quite
irritating for ABI checking.

I am sending it for testing for x86-64 now. Perhaps we can deal with
this by checking ABI with -Os that is a bit less dependent on fine
grained performance decision, like we are seeing here?

Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #15 from pcarlini at suse dot de  2008-03-04 00:09 -------
(In reply to comment #14)
> Hi,
> this is what I get from our thester:
> 
> Differences:
> Tests that now work, but didn't before:
> abi_check
> 
> so it ought to make differnece for i686-linux.

Note however, that the patch also didn't help Geoff's i686-linux tester, just
have a look to gcc-testresults.

> It is quite possible that things differ on 64bit hosts, we are staying
> on quite fragile ground here because in the current cost metric the
> benefits of inlining are very close to costs. Given the nature that
> function call of the wrapped function is a bit chepaer than call of the
> wrapper is quite correct.
> 
> The decision on whether function should be inlined or not depends on
> many things, like overall size, ABI details etc.  I see it is quite
> irritating for ABI checking.

I think we should not mix the two issues, here. The first issue is that, IMO,
the function we are discussing should be inlined, it's very small and we always
inlined it until recently.

The other issue is the ABI check failure. I said issue, but actually it's
really trivial, just matter of tweaking a bit the linker script, making it a
little more tight. I don't think much more is necessary.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[hubicka at ucw dot cz]

------- Comment #16 from hubicka at ucw dot cz  2008-03-04 07:03 -------
Subject: Re:  [4.4 Regression]: FAIL: abi_check

> Note however, that the patch also didn't help Geoff's i686-linux tester, just
> have a look to gcc-testresults.

Sorry, I had two versions of patch and managed to commit the wrong copy.
Sent correct one to ML.  It should be fixed now.
> 
> 
> I think we should not mix the two issues, here. The first issue is that, IMO,
> the function we are discussing should be inlined, it's very small and we always
> inlined it until recently.

The point I wanted to make is that inliner when knowing to be inlining a
cold call (because it was hinted so by __builtin_expect) is correctly a
lot more sellective.  Basically anything that expands to function call
and some extra code around is a loss for code size inlining.

Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262

[Bug c++/35262] [4.4 Regression]: FAIL: abi_check

[pcarlini at suse dot de]

------- Comment #17 from pcarlini at suse dot de  2008-03-04 11:04 -------
(In reply to comment #16)
> Sorry, I had two versions of patch and managed to commit the wrong copy.
> Sent correct one to ML.  It should be fixed now.

Indeed, it's fixed! Many, many thanks!

> The point I wanted to make is that inliner when knowing to be inlining a
> cold call (because it was hinted so by __builtin_expect) is correctly a
> lot more sellective.  Basically anything that expands to function call
> and some extra code around is a loss for code size inlining.

I see now. I was missing the cold call (__builtin_expect) specifics. Thanks for
the explanation.


-- 

pcarlini at suse dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35262