[PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Charles Baylis]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

Hi

This patch is an attempt to fix
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67280. I have written up
an analysis of the bug there.

When cgraph_node::create_wrapper() updates the callgraph for the new
function, it sets the can_throw_external flag to false, even when
wrapping a function which can throw. This causes the ipa-pure-const
phase to mark the wrapper function as nothrow which results in
incorrect unwinding tables. (more details on bugzilla)

The attached patch addresses the problem in
cgraph_node::create_wrapper(). A slightly more general approach would
be to change symbol_table::create_edge() so that it checks
TREE_NOTHROW(callee->decl) when call_stmt is NULL.

This patch passed make check with no new regressions on gcc-5-branch
on arm-linux-gnueabihf using qemu.

I will do a bootstrap on ARM hardware over the weekend. Do I also need
to test x86_64?

I plan to add a test case, but it seems it's worth getting review for
the approach in the mean time.

Thanks,
Charles


gcc/ChangeLog:

2015-08-28  Charles Baylis  <charles.baylis@linaro.org>

        * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
        in new callgraph edge.

[-- Attachment #2: 0001-fix-up-can_throw_external-in-cgraph_node-create_wrap.patch --]
[-- Type: application/x-download, Size: 961 bytes --]

Re: [PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Jeff Law]

On 08/28/2015 09:03 AM, Charles Baylis wrote:
> Hi
>
> This patch is an attempt to fix
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67280. I have written up
> an analysis of the bug there.
>
> When cgraph_node::create_wrapper() updates the callgraph for the new
> function, it sets the can_throw_external flag to false, even when
> wrapping a function which can throw. This causes the ipa-pure-const
> phase to mark the wrapper function as nothrow which results in
> incorrect unwinding tables. (more details on bugzilla)
Seems clearly wrong.  I wonder if there are other properties that should 
be set but aren't in the thunk.

>
> The attached patch addresses the problem in
> cgraph_node::create_wrapper(). A slightly more general approach would
> be to change symbol_table::create_edge() so that it checks
> TREE_NOTHROW(callee->decl) when call_stmt is NULL.
I'm not well versed in the cgraph code -- my worry with this approach 
would be that the wrapper's state is inconsistent with what the wrapper 
can call.  It seems cleaner to make sure these various flags are correct 
when we create the wrapper.

>
> This patch passed make check with no new regressions on gcc-5-branch
> on arm-linux-gnueabihf using qemu.
>
> I will do a bootstrap on ARM hardware over the weekend. Do I also need
> to test x86_64?
A bootstrap on your native target of choice is sufficient.

>
> I plan to add a test case, but it seems it's worth getting review for
> the approach in the mean time.
>
> Thanks,
> Charles
>
>
> gcc/ChangeLog:
>
> 2015-08-28  Charles Baylis  <charles.baylis@linaro.org>
>
>          * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
>          in new callgraph edge.
Ultimately Jan's call.

Jeff

Re: [PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Jan Hubicka]

> >This patch is an attempt to fix
> >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67280. I have written up
> >an analysis of the bug there.
> >
> >When cgraph_node::create_wrapper() updates the callgraph for the new
> >function, it sets the can_throw_external flag to false, even when
> >wrapping a function which can throw. This causes the ipa-pure-const
> >phase to mark the wrapper function as nothrow which results in
Oops...
> >incorrect unwinding tables. (more details on bugzilla)
> Seems clearly wrong.  I wonder if there are other properties that
> should be set but aren't in the thunk.

can_throw_external seems to be only one.
> 
> >
> >The attached patch addresses the problem in
> >cgraph_node::create_wrapper(). A slightly more general approach would
> >be to change symbol_table::create_edge() so that it checks
> >TREE_NOTHROW(callee->decl) when call_stmt is NULL.
> I'm not well versed in the cgraph code -- my worry with this
> approach would be that the wrapper's state is inconsistent with what
> the wrapper can call.  It seems cleaner to make sure these various
> flags are correct when we create the wrapper.

It kind of sucks that one needs to mind this flag each time one creates edge,
but setting the value in create_edge is not quite correct as that one does not
have any information on where the call appears and if the exception is not handled
locally.
> >gcc/ChangeLog:
> >
> >2015-08-28  Charles Baylis  <charles.baylis@linaro.org>
> >
> >         * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
> >         in new callgraph edge.
> Ultimately Jan's call.

This is OK.
Thanks for looking into this!

Honza
> 
> Jeff

Re: [PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Charles Baylis]

On 2 September 2015 at 13:09, Jan Hubicka <hubicka@ucw.cz> wrote:
> It kind of sucks that one needs to mind this flag each time one creates edge,
> but setting the value in create_edge is not quite correct as that one does not
> have any information on where the call appears and if the exception is not handled
> locally.

OK.

>> >gcc/ChangeLog:
>> >
>> >2015-08-28  Charles Baylis  <charles.baylis@linaro.org>
>> >
>> >         * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
>> >         in new callgraph edge.
>> Ultimately Jan's call.
>
> This is OK.
> Thanks for looking into this!

Thanks for the review!

FWIW, the patch successfully bootstrapped on arm-linux-gnueabihf

Committed to trunk as r227407.

Are you happy for me to backport to gcc-5-branch?

Charles

Re: [PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Ramana Radhakrishnan]

On Mon, Sep 7, 2015 at 9:35 AM, Charles Baylis
<charles.baylis@linaro.org> wrote:
> On 2 September 2015 at 13:09, Jan Hubicka <hubicka@ucw.cz> wrote:
>> It kind of sucks that one needs to mind this flag each time one creates edge,
>> but setting the value in create_edge is not quite correct as that one does not
>> have any information on where the call appears and if the exception is not handled
>> locally.
>
> OK.
>
>>> >gcc/ChangeLog:
>>> >
>>> >2015-08-28  Charles Baylis  <charles.baylis@linaro.org>
>>> >
>>> >         * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
>>> >         in new callgraph edge.
>>> Ultimately Jan's call.
>>
>> This is OK.
>> Thanks for looking into this!
>
> Thanks for the review!
>
> FWIW, the patch successfully bootstrapped on arm-linux-gnueabihf

>
> Committed to trunk as r227407.

Missing PR number in Changelog - please fix the changelog  entry and
for your sins add a link to the revision (http://gcc.gnu.org/r227407)
in the BZ entry :)

Ramana

Re: [PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Charles Baylis]

On 7 September 2015 at 09:35, Charles Baylis <charles.baylis@linaro.org> wrote:
>>> >gcc/ChangeLog:
>>> >
>>> >2015-08-28  Charles Baylis  <charles.baylis@linaro.org>
>>> >
>>> >         * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
>>> >         in new callgraph edge.
>
> Committed to trunk as r227407.
>
> Are you happy for me to backport to gcc-5-branch?

Hi Jan,

I'd still like to backport this patch to gcc 5. Is that OK

Thanks
Charles

Re: [PATCH] [ARM, Callgraph] Fix PR67280: function incorrectly marked as nothrow

[Charles Baylis]

Backported r227407 to gcc-5-branch following approval on IRC. The
patch applied without conflicts.

2015-11-16  Charles Baylis  <charles.baylis@linaro.org>

        Backport from mainline r227407
        PR ipa/67280
        * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
        in new callgraph edge.



On 20 September 2015 at 23:53, Charles Baylis <charles.baylis@linaro.org> wrote:
> On 7 September 2015 at 09:35, Charles Baylis <charles.baylis@linaro.org> wrote:
>>>> >gcc/ChangeLog:
>>>> >
>>>> >2015-08-28  Charles Baylis  <charles.baylis@linaro.org>
>>>> >
>>>> >         * cgraphunit.c (cgraph_node::create_wrapper): Set can_throw_external
>>>> >         in new callgraph edge.
>>
>> Committed to trunk as r227407.
>>
>> Are you happy for me to backport to gcc-5-branch?
>
> Hi Jan,
>
> I'd still like to backport this patch to gcc 5. Is that OK
>
> Thanks
> Charles