[PATCH] Fix PR80030
Bootstrapped and tested on x86_64-unknown-linux-gnu, applied. Richard. 2017-03-14 Richard Biener PR tree-optimization/80030 * tree-vect-stmts.c (vectorizable_store): Plug memleak. Index: gcc/tree-vect-stmts.c === --- gcc/tree-vect-stmts.c (revision 246082) +++ gcc/tree-vect-stmts.c (working copy) @@ -6125,6 +6125,8 @@ vectorizable_store (gimple *stmt, gimple if (slp) break; } + + vec_oprnds.release (); return true; }
Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
On Tue, Mar 14, 2017 at 6:32 AM, NightStrike wrote: > On Mon, Mar 13, 2017 at 5:01 AM, Janne Blomqvist > wrote: >> On Sun, Mar 12, 2017 at 10:46 PM, Janne Blomqvist >> wrote: >>> On Sun, Mar 12, 2017 at 7:26 PM, NightStrike wrote: On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist wrote: > Don't try to use rand_s on CYGWIN > > CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is > defined even though rand_s is not available. Thus add an extra check > for __CYGWIN__. > > Thanks to Tim Prince and Nightstrike for bringing this issue to my > attention. > > Committed as r245755. > > 2017-02-27 Janne Blomqvist > > * intrinsics/random.c (getosrandom): Don't try to use rand_s on > CYGWIN. 1) There was no patch attached to the email. >>> >>> Oh, sorry. Well, you can see it at >>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=245755 > > Thanks. You know, this is actually better than attaching a patch (as > a general thing for emails sent after things are already committed), > as there can be no difference between what was emailed and what was > committed. > 2) As mentioned on IRC, I don't think this is the right fix. The real problem is that time_1.h includes windows.h on CYGWIN, which it shouldn't be doing. This then pollutes the translation unit with all sorts of MINGW-isms that aren't exactly appropriate for cygwin. Removing the include in time_1.h and then adjusting a few ifdefs in system_clock.c that also had the same bug fixes the problem. The testsuite is running right now on this. >>> >>> It used to be something like that, but IIRC there were some complaints >>> about SYSTEM_CLOCK on cygwin that were due to the cygwin >>> clock_gettime() or something like that, and after some discussion with >>> someone who knows something about cygwin/mingw (since you apparently >>> have no memory of it, I guess it was Kai), it was decided to use >>> GetTickCount & QPC also on cygwin. >> >> I searched a bit, and it seems the culprit is the thread starting at >> >> https://gcc.gnu.org/ml/fortran/2013-04/msg00033.html >> >> So it turned out that clock_gettime(CLOCK_MONOTONIC, ...) always >> returned 0 on cygwin, hence the code was changed to use the windows >> API functions GetTickCount and QPC also on cygwin at >> >> https://gcc.gnu.org/ml/fortran/2013-04/msg00124.html >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919 > > That all led me to this thread: > > https://cygwin.com/ml/cygwin/2013-04/msg00184.html > > which led me to: > > https://cygwin.com/ml/cygwin/2013-04/msg00191.html > > where Corinna fixed Angelo's original issue that sparked the whole > thing. So, from my perspective, Cygwin hasn't had this problem in > about 4 years. > > To be complete, though, I took Angelo's original code and compiled it > with a GCC built with the change I suggested, and I received this: > > $ ./a.exe >9.50646996E-02 0.435180306 0.939791977 0.851783216 > 0.308901191 0.447312355 0.766363919 0.163527727 > 1.25432014E-02 > > $ ./a.exe > 0.445786893 9.30446386E-03 0.880381405 0.123394549 > 1.23693347E-02 0.485788047 0.623361290 0.921140671 > 0.119319797 > > $ ./a.exe > 0.378171027 0.439836979 0.440582573 1.17767453E-02 > 0.427448869 0.530438244 0.182108700 0.147965968 > 0.668991745 > > $ ./a.exe >2.73125172E-02 0.916011810 0.854288757 0.913502872 > 0.508077919 0.210798383 8.76839161E-02 0.120695710 > 0.337186754 > > > I then tried Janus' enhanced version from > https://gcc.gnu.org/ml/fortran/2013-04/msg00034.html > > $ ./a.exe > n= 33 > clock: 744091787 > seed:744091787 744091824 744091861 744091898 744091935 > 744091972 744092009 744092046 744092083 744092120 744092157 > 744092194 744092231 744092268 744092305 744092342 744092379 > 744092416 744092453 744092490 744092527 744092564 > 744092601 744092638 744092675 744092712 744092749 744092786 > 744092823 744092860 744092897 744092934 744092971 > random: 0.801897824 0.180594921 0.280960143 > 8.65536928E-03 0.122029424 0.473693788 0.161536098 > 0.228073180 0.441518903 > > $ ./a.exe > n= 33 > clock: 744093409 > seed:744093409 744093446 744093483 744093520 744093557 > 744093594 744093631 744093668 744093705 744093742 744093779 > 744093816 744093853 744093890 744093927 744093964 744094001 > 744094038 744094075 744094112 744094149 744094186 > 744094223 744094260 744094297 744094334 744094371 744094408 > 744094445 744094482 744094519 744094556 744094593 > random: 0.164107382 0.762304962 0.511664748 > 0.700617492 0.692375600 0.207925439 0.920203805 > 0.160881400 0.339902878 > > $ ./a.exe > n= 33 > cloc
Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)
Ping.
On Tue, Mar 07, 2017 at 06:10:48PM +0100, Marek Polacek wrote:
> In this testcase we have
> C c = bar (X{1});
> which store_init_value sees as
> c = TARGET_EXPR .n=(&)->i}>)>
> i.e. we're initializing "c" with a TARGET_EXPR. We call replace_placeholders
> that walks the whole tree to substitute the placeholders. Eventually we find
> the nested but that's for another object, so we
> crash. Seems that we shouldn't have stepped into the second TARGET_EXPR at
> all; it has nothing to with "c", it's bar's argument.
>
> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> placeholders in function arguments to cp_gimplify_init_expr which calls
> replace_placeholders for constructors. Not sure if it's enough to handle
> CALL_EXPRs like this, anything else?
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk and 6?
>
> 2017-03-07 Marek Polacek
>
> PR c++/79937 - ICE in replace_placeholders_r
> * tree.c (replace_placeholders_r): Don't walk into CALL_EXPRs.
>
> * g++.dg/cpp1y/nsdmi-aggr7.C: New test.
>
> diff --git gcc/cp/tree.c gcc/cp/tree.c
> index d3c63b8..6a4f065 100644
> --- gcc/cp/tree.c
> +++ gcc/cp/tree.c
> @@ -2751,6 +2751,11 @@ replace_placeholders_r (tree* t, int* walk_subtrees,
> void* data_)
>
>switch (TREE_CODE (*t))
> {
> +case CALL_EXPR:
> + /* Don't mess with placeholders in an unrelated object. */
> + *walk_subtrees = false;
> + break;
> +
> case PLACEHOLDER_EXPR:
>{
> tree x = obj;
> diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> index e69de29..c2fd404 100644
> --- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> +++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> @@ -0,0 +1,21 @@
> +// PR c++/79937
> +// { dg-do compile { target c++14 } }
> +
> +struct C {};
> +
> +struct X {
> + unsigned i;
> + unsigned n = i;
> +};
> +
> +C
> +bar (X)
> +{
> + return {};
> +}
> +
> +void
> +foo ()
> +{
> + C c = bar (X{1});
> +}
>
> Marek
Marek
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/13/2017 04:16 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:53 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:01 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Richard Biener wrote:
>
>> On Fri, 10 Mar 2017, Martin Liška wrote:
>>
>>> Hello.
>>>
>>> As briefly discussed in the PR, there are BB that do not correspond to
>>> a real
>>> line in source
>>> code. profile.c emits locations for all BBs that have a gimple statement
>>> belonging to a line.
>>> I hope these should be marked in gcov utility and not added in
>>> --all-block
>>> mode to counts of lines.
>>>
>>> Patch survives make check RUNTESTFLAGS="gcov.exp".
>>>
>>> Thanks for review and feedback.
>>
>> Humm, the patch doesn't seem to change the BBs associated with a line
>> but rather somehow changes how we compute counts (the patch lacks a
>> description of how you arrived at it). I expected the line-to-BB
>> assignment process to be changed (whereever that is...).
Currently, each basic block must belong to a source line. It's how gcov
iterates all blocks (via lines).
>
> Ah, ok, looking at where output_location is called on I see we do not
> assign any line to that block. But still why does
> line->has_block (arc->src) return true?
Good objection! Problematic that 4->5 edge really comes from the same
line.
[0.00%]:
ret_7 = 111;
PROF_edge_counter_10 = __gcov0.UuT[0];
PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
__gcov0.UuT[0] = PROF_edge_counter_11;
[0.00%]:
# ret_1 = PHI
goto ; [0.00%]
>>>
>>> Yes, but that's basically meaningless, unless not all edges come from the
>>> same line? I see nowhere where we'd explicitely assign lines to
>>> edges so it must be gcov "reconstructing" this somewhere.
>>
>> That's why I added the another flag. We stream locations for basic blocks via
>> 'output_location' function. And assignment blocks to lines happens here:
>>
>> static void
>> add_line_counts (coverage_t *coverage, function_t *fn)
>> {
>> ...
>> if (!ix || ix + 1 == fn->num_blocks)
>> /* Entry or exit block */;
>> else if (flag_all_blocks)
>> {
>>line_t *block_line = line;
>>
>>if (!block_line)
>> block_line = &sources[fn->src].lines[fn->line];
>>
>>block->chain = block_line->u.blocks;
>>block_line->u.blocks = block;
>> }
>>
>> where line is always changes when we reach a BB that has a source line
>> assigned. Thus it's changed
>> for BB 4 and that's why BB 5 is added to same line.
>
> Ah, so this means we should "clear" the current line for BB 5 in
> output_location? At least I don't see how your patch may not regress
> some cases where the line wasn't output as an optimization?
The new flag on block is kind of clearing that the BB is artificial and in fact
does not
belong to the line. I didn't want to do a bigger refactoring how blocks are
iterated via lines.
Can you be please more specific about such a case?
Hope Nathan will find time to provide review as he's familiar with content of
gcov.c.
Martin
>
> OTOH I don't know much about gcov format.
>
> Richard.
>
>> Martin
>>
>>>
>>> Richard.
>>>
Martin
>
> Richard.
>
>>>
>>
>>
>
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/13/2017 04:16 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:53 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:01 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Richard Biener wrote:
> >
> >> On Fri, 10 Mar 2017, Martin Liška wrote:
> >>
> >>> Hello.
> >>>
> >>> As briefly discussed in the PR, there are BB that do not correspond
> >>> to a real
> >>> line in source
> >>> code. profile.c emits locations for all BBs that have a gimple
> >>> statement
> >>> belonging to a line.
> >>> I hope these should be marked in gcov utility and not added in
> >>> --all-block
> >>> mode to counts of lines.
> >>>
> >>> Patch survives make check RUNTESTFLAGS="gcov.exp".
> >>>
> >>> Thanks for review and feedback.
> >>
> >> Humm, the patch doesn't seem to change the BBs associated with a line
> >> but rather somehow changes how we compute counts (the patch lacks a
> >> description of how you arrived at it). I expected the line-to-BB
> >> assignment process to be changed (whereever that is...).
>
> Currently, each basic block must belong to a source line. It's how gcov
> iterates all blocks (via lines).
>
> >
> > Ah, ok, looking at where output_location is called on I see we do not
> > assign any line to that block. But still why does
> > line->has_block (arc->src) return true?
>
> Good objection! Problematic that 4->5 edge really comes from the same
> line.
>
> [0.00%]:
> ret_7 = 111;
> PROF_edge_counter_10 = __gcov0.UuT[0];
> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> __gcov0.UuT[0] = PROF_edge_counter_11;
>
> [0.00%]:
> # ret_1 = PHI
> goto ; [0.00%]
> >>>
> >>> Yes, but that's basically meaningless, unless not all edges come from the
> >>> same line? I see nowhere where we'd explicitely assign lines to
> >>> edges so it must be gcov "reconstructing" this somewhere.
> >>
> >> That's why I added the another flag. We stream locations for basic blocks
> >> via
> >> 'output_location' function. And assignment blocks to lines happens here:
> >>
> >> static void
> >> add_line_counts (coverage_t *coverage, function_t *fn)
> >> {
> >> ...
> >> if (!ix || ix + 1 == fn->num_blocks)
> >>/* Entry or exit block */;
> >> else if (flag_all_blocks)
> >>{
> >> line_t *block_line = line;
> >>
> >> if (!block_line)
> >>block_line = &sources[fn->src].lines[fn->line];
> >>
> >> block->chain = block_line->u.blocks;
> >> block_line->u.blocks = block;
> >>}
> >>
> >> where line is always changes when we reach a BB that has a source line
> >> assigned. Thus it's changed
> >> for BB 4 and that's why BB 5 is added to same line.
> >
> > Ah, so this means we should "clear" the current line for BB 5 in
> > output_location? At least I don't see how your patch may not regress
> > some cases where the line wasn't output as an optimization?
>
> The new flag on block is kind of clearing that the BB is artificial and in
> fact does not
> belong to the line. I didn't want to do a bigger refactoring how blocks are
> iterated via lines.
>
> Can you be please more specific about such a case?
in profile.c I see
if (name_differs || line_differs)
{
if (!*offset)
{
*offset = gcov_write_tag (GCOV_TAG_LINES);
gcov_write_unsigned (bb->index);
name_differs = line_differs=true;
}
...
so if line_differs is false we might not output GCOV_TAG_LINES either
because 1) optimization, less stuff output, 2) the block has no line.
Looks like we can't really distinguish those.
Not sure how on the input side we end up associating a BB with
a line if nothing was output for it though.
That is, with your change don't we need
Index: gcc/profile.c
===
--- gcc/profile.c (revision 246082)
+++ gcc/profile.c (working copy)
@@ -941,8 +941,6 @@ output_location (char const *file_name,
name_differs = !prev_file_name || filename_cmp (file_name,
prev_file_name);
line_differs = prev_line != line;
- if (name_differs || line_differs)
-{
if (!*offset)
{
*offset = gcov_write_tag (GCOV_TAG_LINES);
@@ -950,6 +948,9 @@ output_location (char const *file_name,
name_differs = line_differs=true;
}
+ if (name_differs || line_differs)
+{
+
/* If this is a new source file, then output the
file's name to the .bb file. */
if (name_differs)
to resolve this ambiguity? That is, _always_ emit GCOV_TAG_LINES
for a BB? So then a BB w/o GCOV_TAG_LINES does _not_ have any
lines associated.
Richard.
> Hope Nathan will find time to provide review as
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov wrote: > Hello everyone, i have a patch for this issue. Great! > List of implemented functionality: > > 1.Reading .gnu_debuglink section from ELF file: > a. Reading name of debug info file. > b. Verifying crc32 sum. > > 2. Searching for separate debug info file from paths: > a. /usr/lib/debug/path/to/executable > b. /path/to/executable > c. /path/to/executable/.debug > > Assumed that debug info file generated by objcopy from binutils. > > objcopy --only-keep-debug foo foo.debug > strip -g foo > objcopy --add-gnu-debuglink=foo.debug foo Skimming over the patch I noticed you duplicate libiberties xcrc32 functionality. Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature is dwarf + elf specific but proper abstraction / #ifdefing should ensure compiling also succeeds for non-dwarf / non-elf platforms). Leaving actual review to the maintainer (CCed). Thanks, Richard.
Re: [patch, libfortran] [patch, fortran] Fix PR 79956, part two, attempt 2
On 2017.03.13 at 23:24 +0100, Thomas Koenig wrote:
> Hello world,
>
> Following Richard's suggestion, I have implemented the GFC_ASSERT macro
> and used it to (hopefully) silence the ominous warning and allow
> further optimization.
Unfortunately the single GFC_ASSERT isn't enough:
libgfortran/intrinsics/reshape_generic.c: In function ‘reshape_internal’:
libgfortran/intrinsics/reshape_generic.c:257:21: warning: ‘sstride[0]’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
sstride0 = sstride[0] * size;
~~~^~~
The following patch is needed to silence them all.
diff --git a/libgfortran/intrinsics/reshape_generic.c
b/libgfortran/intrinsics/reshape_generic.c
index 43a822f87ae..d7c9e74853a 100644
--- a/libgfortran/intrinsics/reshape_generic.c
+++ b/libgfortran/intrinsics/reshape_generic.c
@@ -66,6 +66,7 @@ reshape_internal (parray *ret, parray *source, shape_type
*shape,
index_type shape_data[GFC_MAX_DIMENSIONS];
rdim = GFC_DESCRIPTOR_EXTENT(shape,0);
+ GFC_ASSERT(rdim>0);
if (rdim != GFC_DESCRIPTOR_RANK(ret))
runtime_error("rank of return array incorrect in RESHAPE intrinsic");
@@ -158,6 +159,10 @@ reshape_internal (parray *ret, parray *source, shape_type
*shape,
source_extent = 1;
sdim = GFC_DESCRIPTOR_RANK (source);
+ /* sdim is always > 0; this lets the compiler optimize more and
+ avoids a warning. */
+ GFC_ASSERT(sdim>0);
+
for (n = 0; n < sdim; n++)
{
index_type se;
@@ -219,6 +224,7 @@ reshape_internal (parray *ret, parray *source, shape_type
*shape,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+ GFC_ASSERT(sdim>0);
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
--
Markus
Re: [PATCH] Fix PR79908
On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
wrote:
> Hi,
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79908 shows a case where
> pass_stdarg ICEs attempting to gimplify a COMPLEX_EXPR with side
> effects as an lvalue. The expression is not addressable, so the
> gimplification fails. This patch says, hey, don't do that!
>
> The resulting GIMPLE looks fine afterward. Bootstrapped and tested
> on powerpc64le-unknown-linux-gnu with no regressions. Is this ok
> for trunk?
>
> Thanks,
> Bill
>
>
> [gcc]
>
> 2017-03-13 Bill Schmidt
>
> PR tree-optimization/79908
> * tree-stdarg.c (expand_ifn_va_arg_1): Don't force something to be
> an lvalue that isn't addressable.
>
> [gcc/testsuite]
>
> 2017-03-13 Bill Schmidt
>
> PR tree-optimization/79908
> * gcc.dg/torture/pr79908.c: New file.
>
>
> Index: gcc/testsuite/gcc.dg/torture/pr79908.c
> ===
> --- gcc/testsuite/gcc.dg/torture/pr79908.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/torture/pr79908.c (working copy)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +
> +/* Used to fail in the stdarg pass before fix for PR79908. */
> +
> +typedef __builtin_va_list __gnuc_va_list;
> +typedef __gnuc_va_list va_list;
> +
> +void testva (int n, ...)
> +{
> + va_list ap;
> + _Complex int i = __builtin_va_arg (ap, _Complex int);
> +}
> Index: gcc/tree-stdarg.c
> ===
> --- gcc/tree-stdarg.c (revision 246109)
> +++ gcc/tree-stdarg.c (working copy)
> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun)
>types. */
> gimplify_assign (lhs, expr, &pre);
> }
> - else
> + else if (is_gimple_addressable (expr))
> gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
This is wrong - we lose side-effects this way and the only reason we gimplify
is to _not_ lose them.
Better is sth like
Index: gcc/tree-stdarg.c
===
--- gcc/tree-stdarg.c (revision 246082)
+++ gcc/tree-stdarg.c (working copy)
@@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun)
gimplify_assign (lhs, expr, &pre);
}
else
- gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
+ gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
input_location = saved_location;
pop_gimplify_context (NULL);
>
> input_location = saved_location;
>
Re: [PATCH] add calls.c to GTFILES in Makefile.in (PR 79936)
On Tue, Mar 14, 2017 at 1:37 AM, Martin Sebor wrote: > Ping: this a P3 regression targeted at 7.0.1. It was found > on x86_64-apple-darwin10.8.0 but affects all targets (even > if it doesn't happen to manifest on them): > > https://gcc.gnu.org/ml/gcc-patches/2017-03/msg00342.html I think you need to include gt-calls.h at the bottom of the file. See other GTFILES > On 03/07/2017 06:03 PM, Martin Sebor wrote: >> >> In bug 79936 - ICE with -Walloc-size-larger-than=32767 the reporter >> encountered an ICE on x86_64-apple-darwin10.8.0 caused by GCC source >> file that implements the warning accessing a global tree without >> having included the file in GTFILES make variable. (Thanks again >> to David Malcolm who helped me root cause this bug.) >> >> The attached patch fixes this by adding calls.c to the variable and >> by including the requisite generated header, gt-calls.h, at the end >> of the source. >> >> Martin > >
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 09:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 04:16 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:53 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:01 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Richard Biener wrote:
>>>
On Fri, 10 Mar 2017, Martin Liška wrote:
> Hello.
>
> As briefly discussed in the PR, there are BB that do not correspond
> to a real
> line in source
> code. profile.c emits locations for all BBs that have a gimple
> statement
> belonging to a line.
> I hope these should be marked in gcov utility and not added in
> --all-block
> mode to counts of lines.
>
> Patch survives make check RUNTESTFLAGS="gcov.exp".
>
> Thanks for review and feedback.
Humm, the patch doesn't seem to change the BBs associated with a line
but rather somehow changes how we compute counts (the patch lacks a
description of how you arrived at it). I expected the line-to-BB
assignment process to be changed (whereever that is...).
>>
>> Currently, each basic block must belong to a source line. It's how gcov
>> iterates all blocks (via lines).
>>
>>>
>>> Ah, ok, looking at where output_location is called on I see we do not
>>> assign any line to that block. But still why does
>>> line->has_block (arc->src) return true?
>>
>> Good objection! Problematic that 4->5 edge really comes from the same
>> line.
>>
>>[0.00%]:
>> ret_7 = 111;
>> PROF_edge_counter_10 = __gcov0.UuT[0];
>> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
>> __gcov0.UuT[0] = PROF_edge_counter_11;
>>
>>[0.00%]:
>> # ret_1 = PHI
>> goto ; [0.00%]
>
> Yes, but that's basically meaningless, unless not all edges come from the
> same line? I see nowhere where we'd explicitely assign lines to
> edges so it must be gcov "reconstructing" this somewhere.
That's why I added the another flag. We stream locations for basic blocks
via
'output_location' function. And assignment blocks to lines happens here:
static void
add_line_counts (coverage_t *coverage, function_t *fn)
{
...
if (!ix || ix + 1 == fn->num_blocks)
/* Entry or exit block */;
else if (flag_all_blocks)
{
line_t *block_line = line;
if (!block_line)
block_line = &sources[fn->src].lines[fn->line];
block->chain = block_line->u.blocks;
block_line->u.blocks = block;
}
where line is always changes when we reach a BB that has a source line
assigned. Thus it's changed
for BB 4 and that's why BB 5 is added to same line.
>>>
>>> Ah, so this means we should "clear" the current line for BB 5 in
>>> output_location? At least I don't see how your patch may not regress
>>> some cases where the line wasn't output as an optimization?
>>
>> The new flag on block is kind of clearing that the BB is artificial and in
>> fact does not
>> belong to the line. I didn't want to do a bigger refactoring how blocks are
>> iterated via lines.
>>
>> Can you be please more specific about such a case?
>
> in profile.c I see
>
> if (name_differs || line_differs)
> {
> if (!*offset)
> {
> *offset = gcov_write_tag (GCOV_TAG_LINES);
> gcov_write_unsigned (bb->index);
> name_differs = line_differs=true;
> }
>
> ...
>
> so if line_differs is false we might not output GCOV_TAG_LINES either
> because 1) optimization, less stuff output, 2) the block has no line.
> Looks like we can't really distinguish those.
Agree with that.
>
> Not sure how on the input side we end up associating a BB with
> a line if nothing was output for it though.
>
> That is, with your change don't we need
>
> Index: gcc/profile.c
> ===
> --- gcc/profile.c (revision 246082)
> +++ gcc/profile.c (working copy)
> @@ -941,8 +941,6 @@ output_location (char const *file_name,
>name_differs = !prev_file_name || filename_cmp (file_name,
> prev_file_name);
>line_differs = prev_line != line;
>
> - if (name_differs || line_differs)
> -{
>if (!*offset)
> {
> *offset = gcov_write_tag (GCOV_TAG_LINES);
> @@ -950,6 +948,9 @@ output_location (char const *file_name,
> name_differs = line_differs=true;
> }
>
> + if (name_differs || line_differs)
> +{
> +
>/* If this is a new source file, then output the
> file's name to the .bb file. */
>if (name_differs)
>
> to resolve this ambiguity? That is, _
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 09:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 04:16 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:53 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:01 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Richard Biener wrote:
> >>>
> On Fri, 10 Mar 2017, Martin Liška wrote:
>
> > Hello.
> >
> > As briefly discussed in the PR, there are BB that do not correspond
> > to a real
> > line in source
> > code. profile.c emits locations for all BBs that have a gimple
> > statement
> > belonging to a line.
> > I hope these should be marked in gcov utility and not added in
> > --all-block
> > mode to counts of lines.
> >
> > Patch survives make check RUNTESTFLAGS="gcov.exp".
> >
> > Thanks for review and feedback.
>
> Humm, the patch doesn't seem to change the BBs associated with a line
> but rather somehow changes how we compute counts (the patch lacks a
> description of how you arrived at it). I expected the line-to-BB
> assignment process to be changed (whereever that is...).
> >>
> >> Currently, each basic block must belong to a source line. It's how gcov
> >> iterates all blocks (via lines).
> >>
> >>>
> >>> Ah, ok, looking at where output_location is called on I see we do not
> >>> assign any line to that block. But still why does
> >>> line->has_block (arc->src) return true?
> >>
> >> Good objection! Problematic that 4->5 edge really comes from the same
> >> line.
> >>
> >>[0.00%]:
> >> ret_7 = 111;
> >> PROF_edge_counter_10 = __gcov0.UuT[0];
> >> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> >> __gcov0.UuT[0] = PROF_edge_counter_11;
> >>
> >>[0.00%]:
> >> # ret_1 = PHI
> >> goto ; [0.00%]
> >
> > Yes, but that's basically meaningless, unless not all edges come from
> > the
> > same line? I see nowhere where we'd explicitely assign lines to
> > edges so it must be gcov "reconstructing" this somewhere.
>
> That's why I added the another flag. We stream locations for basic
> blocks via
> 'output_location' function. And assignment blocks to lines happens here:
>
> static void
> add_line_counts (coverage_t *coverage, function_t *fn)
> {
> ...
> if (!ix || ix + 1 == fn->num_blocks)
> /* Entry or exit block */;
> else if (flag_all_blocks)
> {
> line_t *block_line = line;
>
> if (!block_line)
> block_line = &sources[fn->src].lines[fn->line];
>
> block->chain = block_line->u.blocks;
> block_line->u.blocks = block;
> }
>
> where line is always changes when we reach a BB that has a source line
> assigned. Thus it's changed
> for BB 4 and that's why BB 5 is added to same line.
> >>>
> >>> Ah, so this means we should "clear" the current line for BB 5 in
> >>> output_location? At least I don't see how your patch may not regress
> >>> some cases where the line wasn't output as an optimization?
> >>
> >> The new flag on block is kind of clearing that the BB is artificial and in
> >> fact does not
> >> belong to the line. I didn't want to do a bigger refactoring how blocks
> >> are iterated via lines.
> >>
> >> Can you be please more specific about such a case?
> >
> > in profile.c I see
> >
> > if (name_differs || line_differs)
> > {
> > if (!*offset)
> > {
> > *offset = gcov_write_tag (GCOV_TAG_LINES);
> > gcov_write_unsigned (bb->index);
> > name_differs = line_differs=true;
> > }
> >
> > ...
> >
> > so if line_differs is false we might not output GCOV_TAG_LINES either
> > because 1) optimization, less stuff output, 2) the block has no line.
> > Looks like we can't really distinguish those.
>
> Agree with that.
>
> >
> > Not sure how on the input side we end up associating a BB with
> > a line if nothing was output for it though.
> >
> > That is, with your change don't we need
> >
> > Index: gcc/profile.c
> > ===
> > --- gcc/profile.c (revision 246082)
> > +++ gcc/profile.c (working copy)
> > @@ -941,8 +941,6 @@ output_location (char const *file_name,
> >name_differs = !prev_file_name || filename_cmp (file_name,
> > prev_file_name);
> >line_differs = prev_line != line;
> >
> > - if (name_differs || line_differs)
> > -{
> >if (!*offset)
> > {
> > *offset = gcov_write_tag (GCOV_TAG_LINES);
> > @@ -950,6 +948,9 @@ output_location (ch
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
On 14.03.2017 09:27, Richard Biener wrote: > On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov > wrote: >> Hello everyone, i have a patch for this issue. > > Great! > >> List of implemented functionality: >> >> 1.Reading .gnu_debuglink section from ELF file: >> a. Reading name of debug info file. >> b. Verifying crc32 sum. >> >> 2. Searching for separate debug info file from paths: >> a. /usr/lib/debug/path/to/executable >> b. /path/to/executable >> c. /path/to/executable/.debug >> >> Assumed that debug info file generated by objcopy from binutils. >> >> objcopy --only-keep-debug foo foo.debug >> strip -g foo >> objcopy --add-gnu-debuglink=foo.debug foo These days using the build-id for separate debug info seems to be the new way of doing that, so adding support for the build-id method would be useful. See PR binutils/20876 for how binutils is doing that. Matthias
Re: [PATCH] [ADA] Fix bootstrap failure on mips64el-linux-gnuabi64
On 13.03.2017 18:26, James Cowgill wrote: > Hi, > > On 11/03/17 12:11, Arnaud Charlet wrote: >>> This patch fixes an error caused by my changing of the signal constants >>> on MIPS in r244026. While that patch worked on mipsel, ada fails to >>> bootstrap with it on mips64el with the error: >>> >>> s-osinte.ads:610:07: component "sa_flags" overlaps "sa_handler" at >>> line 608 >>> ../gcc-interface/Makefile:297: recipe for target 'a-dispat.o' failed >>> make[9]: *** [a-dispat.o] Error 1 >>> >>> The fix is to adjust the size of sa_flags in struct_sigaction from an >>> unsigned long to an int. I checked the glibc sources and sa_flags is an >>> int on all Linux arches. >> >> Patch is OK, thanks. > > Can you commit it for me? I committed the patch. Matthias >>> gcc/ada/Changelog: >>> >>> 2017-03-10 James Cowgill >>> >>> * s-osinte-linux.ads (struct_sigaction): Use correct type for >>> sa_flags. >
Re: [RFC][PATCH][AArch64] Improve generic branch cost
On Thu, Mar 09, 2017 at 02:06:16PM -0800, Andrew Pinski wrote: > On Thu, Mar 9, 2017 at 6:42 AM, Wilco Dijkstra wrote: > > Hi, > > > > Recently we've put a lot of effort into improving ifcvt to use CSEL on > > AArch64. In https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01639.html > > James determined the best value for AArch64 code generation. This was before the rewrite to the ifcvt costs which I made earlier in the GCC 7 release cycle. But I think 1,3 is about right, and I'd be happy to see us take that direction for "generic". I'd like to hear comments from the Exynos-M1, Falkor and xgene-1 subtarget contributors, particularly as these targets use generic_branch_costs for their subtarget-sepcific tuning. It may be that your patch needs to preserve the 2,2 setting for such cores even if the generic target does move to 1,3. At this stage in the release, this patch will have to wait for GCC 8 regardless of any comments received. I'd suggest that when we do think about this for GCC 8, we might want to take a wider look at the "generic" tunings, any opinions from other subtarget contributors, or the other AArch64 maintainers as to further changes they would advocate for would be welcome. > I have no objections. In fact thunderx2t99's branch_cost is 1,3. I > had not looked into improving thunderx branch cost yet but that might > be because I have local patches that improve phiopt for doing ifcvt > earlier. Also my phiopt change does not have a cost model either so > using csel more is good for thunderx 1 and ThunderX 2. Thanks for the comments, James
Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).
On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška wrote: > Hello. > > This is a small follow-up patch, where local.local flag should be also dropped > for a default implementation. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? I see we have clered the flag on the clones this way. But isn't it bogus to have it set in the first place? That is, isn't analysis sofar given bogus info? Shouldn't we instead fix local_p to not mark functions with MV attribute local in the first place? Richard. > Martin
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 10:12 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 09:13 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 04:16 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:53 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:01 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Richard Biener wrote:
>
>> On Fri, 10 Mar 2017, Martin Liška wrote:
>>
>>> Hello.
>>>
>>> As briefly discussed in the PR, there are BB that do not correspond
>>> to a real
>>> line in source
>>> code. profile.c emits locations for all BBs that have a gimple
>>> statement
>>> belonging to a line.
>>> I hope these should be marked in gcov utility and not added in
>>> --all-block
>>> mode to counts of lines.
>>>
>>> Patch survives make check RUNTESTFLAGS="gcov.exp".
>>>
>>> Thanks for review and feedback.
>>
>> Humm, the patch doesn't seem to change the BBs associated with a line
>> but rather somehow changes how we compute counts (the patch lacks a
>> description of how you arrived at it). I expected the line-to-BB
>> assignment process to be changed (whereever that is...).
Currently, each basic block must belong to a source line. It's how gcov
iterates all blocks (via lines).
>
> Ah, ok, looking at where output_location is called on I see we do not
> assign any line to that block. But still why does
> line->has_block (arc->src) return true?
Good objection! Problematic that 4->5 edge really comes from the same
line.
[0.00%]:
ret_7 = 111;
PROF_edge_counter_10 = __gcov0.UuT[0];
PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
__gcov0.UuT[0] = PROF_edge_counter_11;
[0.00%]:
# ret_1 = PHI
goto ; [0.00%]
>>>
>>> Yes, but that's basically meaningless, unless not all edges come from
>>> the
>>> same line? I see nowhere where we'd explicitely assign lines to
>>> edges so it must be gcov "reconstructing" this somewhere.
>>
>> That's why I added the another flag. We stream locations for basic
>> blocks via
>> 'output_location' function. And assignment blocks to lines happens here:
>>
>> static void
>> add_line_counts (coverage_t *coverage, function_t *fn)
>> {
>> ...
>> if (!ix || ix + 1 == fn->num_blocks)
>> /* Entry or exit block */;
>> else if (flag_all_blocks)
>> {
>>line_t *block_line = line;
>>
>>if (!block_line)
>> block_line = &sources[fn->src].lines[fn->line];
>>
>>block->chain = block_line->u.blocks;
>>block_line->u.blocks = block;
>> }
>>
>> where line is always changes when we reach a BB that has a source line
>> assigned. Thus it's changed
>> for BB 4 and that's why BB 5 is added to same line.
>
> Ah, so this means we should "clear" the current line for BB 5 in
> output_location? At least I don't see how your patch may not regress
> some cases where the line wasn't output as an optimization?
The new flag on block is kind of clearing that the BB is artificial and in
fact does not
belong to the line. I didn't want to do a bigger refactoring how blocks
are iterated via lines.
Can you be please more specific about such a case?
>>>
>>> in profile.c I see
>>>
>>> if (name_differs || line_differs)
>>> {
>>> if (!*offset)
>>> {
>>> *offset = gcov_write_tag (GCOV_TAG_LINES);
>>> gcov_write_unsigned (bb->index);
>>> name_differs = line_differs=true;
>>> }
>>>
>>> ...
>>>
>>> so if line_differs is false we might not output GCOV_TAG_LINES either
>>> because 1) optimization, less stuff output, 2) the block has no line.
>>> Looks like we can't really distinguish those.
>>
>> Agree with that.
>>
>>>
>>> Not sure how on the input side we end up associating a BB with
>>> a line if nothing was output for it though.
>>>
>>> That is, with your change don't we need
>>>
>>> Index: gcc/profile.c
>>> ===
>>> --- gcc/profile.c (revision 246082)
>>> +++ gcc/profile.c (working copy)
>>> @@ -941,8 +941,6 @@ output_location (char const *file_name,
>>>name_differs = !prev_file_name || filename_cmp (file_name,
>>> prev_file_name);
>>>line_differs = prev_line != line;
>>>
>>> - if (name_differs || line_differs)
>>> -{
>>>if (!*offset)
>>> {
>>> *offset = gcov_write_tag (GCOV_TAG_LINES);
Re: [PATCH 1/7] Add missing punctuation to message (PR driver/79875)
On 11 March 2017 11:28:46 CET, Roland Illig wrote: >Am 10.03.2017 um 05:12 schrieb Martin Sebor: >> I have just an observation/question here for future consideration. >> If this sort of diagnostic is common (I count 23 instances of it) >> or if it is expected to become common, would it make sense to add >> a directive for it to the pretty printer to ensure consistency? >> I.e., to automatically prepend "; did you mean" and append the >> "?" to the new directive? > >As a translator, I'm not too concerned about this one, although it >looks >very similar to the repeated "%qs requires %qs", which often appears >for >command line options. > >My main argument is that the "did you mean" is not just a name for a >thing, but it is part of the sentence grammar. To me that feels like >too >much magic for a rarely used case (compared to %D or %qE or %qT, which >appear so often that I'm now very familiar with them). > >Also, when adding a placeholder like %H for it, there would be no >surrounding whitespace, similar to %K. That confused me as a translator >when I first saw it, since it is not nicely documented; neither in the >Gettext manual >(https://www.gnu.org/software/gettext/manual/gettext.html#gcc_002dinternal_002dformat) >nor in the GCC files implementing them. There should be at least some >examples of what each placeholder typically expands to. I think I suggested this a when the hints were introduced but Joseph declined the idea back then, FYI. thanks,
Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).
On 03/14/2017 10:46 AM, Richard Biener wrote: > On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška wrote: >> Hello. >> >> This is a small follow-up patch, where local.local flag should be also >> dropped >> for a default implementation. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > > I see we have clered the flag on the clones this way. But isn't it > bogus to have > it set in the first place? That is, isn't analysis sofar given bogus info? Yes, I did it for clones. Reason why we mark it is that local flag is set by pass_ipa_function_and_variable_visibility pass, which runs before MV pass. I can imagine MV can bail out for a non-trivial reason and the visibility pass should somehow simulate and predict what happens in the MV pass? Martin > > Shouldn't we instead fix local_p to not mark functions with MV attribute local > in the first place? > > Richard. > >> Martin
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 10:12 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 09:13 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 04:16 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:53 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:01 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Richard Biener wrote:
> >
> >> On Fri, 10 Mar 2017, Martin Liška wrote:
> >>
> >>> Hello.
> >>>
> >>> As briefly discussed in the PR, there are BB that do not
> >>> correspond to a real
> >>> line in source
> >>> code. profile.c emits locations for all BBs that have a gimple
> >>> statement
> >>> belonging to a line.
> >>> I hope these should be marked in gcov utility and not added in
> >>> --all-block
> >>> mode to counts of lines.
> >>>
> >>> Patch survives make check RUNTESTFLAGS="gcov.exp".
> >>>
> >>> Thanks for review and feedback.
> >>
> >> Humm, the patch doesn't seem to change the BBs associated with a
> >> line
> >> but rather somehow changes how we compute counts (the patch lacks a
> >> description of how you arrived at it). I expected the line-to-BB
> >> assignment process to be changed (whereever that is...).
>
> Currently, each basic block must belong to a source line. It's how
> gcov
> iterates all blocks (via lines).
>
> >
> > Ah, ok, looking at where output_location is called on I see we do
> > not
> > assign any line to that block. But still why does
> > line->has_block (arc->src) return true?
>
> Good objection! Problematic that 4->5 edge really comes from the
> same line.
>
> [0.00%]:
> ret_7 = 111;
> PROF_edge_counter_10 = __gcov0.UuT[0];
> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> __gcov0.UuT[0] = PROF_edge_counter_11;
>
> [0.00%]:
> # ret_1 = PHI
> goto ; [0.00%]
> >>>
> >>> Yes, but that's basically meaningless, unless not all edges come from
> >>> the
> >>> same line? I see nowhere where we'd explicitely assign lines to
> >>> edges so it must be gcov "reconstructing" this somewhere.
> >>
> >> That's why I added the another flag. We stream locations for basic
> >> blocks via
> >> 'output_location' function. And assignment blocks to lines happens
> >> here:
> >>
> >> static void
> >> add_line_counts (coverage_t *coverage, function_t *fn)
> >> {
> >> ...
> >> if (!ix || ix + 1 == fn->num_blocks)
> >>/* Entry or exit block */;
> >> else if (flag_all_blocks)
> >>{
> >> line_t *block_line = line;
> >>
> >> if (!block_line)
> >>block_line = &sources[fn->src].lines[fn->line];
> >>
> >> block->chain = block_line->u.blocks;
> >> block_line->u.blocks = block;
> >>}
> >>
> >> where line is always changes when we reach a BB that has a source line
> >> assigned. Thus it's changed
> >> for BB 4 and that's why BB 5 is added to same line.
> >
> > Ah, so this means we should "clear" the current line for BB 5 in
> > output_location? At least I don't see how your patch may not regress
> > some cases where the line wasn't output as an optimization?
>
> The new flag on block is kind of clearing that the BB is artificial and
> in fact does not
> belong to the line. I didn't want to do a bigger refactoring how blocks
> are iterated via lines.
>
> Can you be please more specific about such a case?
> >>>
> >>> in profile.c I see
> >>>
> >>> if (name_differs || line_differs)
> >>> {
> >>> if (!*offset)
> >>> {
> >>> *offset = gcov_write_tag (GCOV_TAG_LINES);
> >>> gcov_write_unsigned (bb->index);
> >>> name_differs = line_differs=true;
> >>> }
> >>>
> >>> ...
> >>>
> >>> so if line_differs is false we might not output GCOV_TAG_LINES either
> >>> because 1) optimization, less stuff output, 2) the block has no line.
> >>> Looks like we can't really distinguish those.
> >>
> >> Agree with that.
> >>
> >>>
> >>> Not sure how on the input side we end up associating a BB with
> >>> a line if nothing was output for it though.
> >>>
> >>> That is, with your change don't we need
> >>>
> >>> Index: gcc/profile.c
> >>> ===
> >>> --- gcc/profile.c (revision 24
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 11:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 10:12 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 09:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 04:16 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:53 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:01 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Richard Biener wrote:
>>>
On Fri, 10 Mar 2017, Martin Liška wrote:
> Hello.
>
> As briefly discussed in the PR, there are BB that do not
> correspond to a real
> line in source
> code. profile.c emits locations for all BBs that have a gimple
> statement
> belonging to a line.
> I hope these should be marked in gcov utility and not added in
> --all-block
> mode to counts of lines.
>
> Patch survives make check RUNTESTFLAGS="gcov.exp".
>
> Thanks for review and feedback.
Humm, the patch doesn't seem to change the BBs associated with a
line
but rather somehow changes how we compute counts (the patch lacks a
description of how you arrived at it). I expected the line-to-BB
assignment process to be changed (whereever that is...).
>>
>> Currently, each basic block must belong to a source line. It's how
>> gcov
>> iterates all blocks (via lines).
>>
>>>
>>> Ah, ok, looking at where output_location is called on I see we do
>>> not
>>> assign any line to that block. But still why does
>>> line->has_block (arc->src) return true?
>>
>> Good objection! Problematic that 4->5 edge really comes from the
>> same line.
>>
>>[0.00%]:
>> ret_7 = 111;
>> PROF_edge_counter_10 = __gcov0.UuT[0];
>> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
>> __gcov0.UuT[0] = PROF_edge_counter_11;
>>
>>[0.00%]:
>> # ret_1 = PHI
>> goto ; [0.00%]
>
> Yes, but that's basically meaningless, unless not all edges come from
> the
> same line? I see nowhere where we'd explicitely assign lines to
> edges so it must be gcov "reconstructing" this somewhere.
That's why I added the another flag. We stream locations for basic
blocks via
'output_location' function. And assignment blocks to lines happens
here:
static void
add_line_counts (coverage_t *coverage, function_t *fn)
{
...
if (!ix || ix + 1 == fn->num_blocks)
/* Entry or exit block */;
else if (flag_all_blocks)
{
line_t *block_line = line;
if (!block_line)
block_line = &sources[fn->src].lines[fn->line];
block->chain = block_line->u.blocks;
block_line->u.blocks = block;
}
where line is always changes when we reach a BB that has a source line
assigned. Thus it's changed
for BB 4 and that's why BB 5 is added to same line.
>>>
>>> Ah, so this means we should "clear" the current line for BB 5 in
>>> output_location? At least I don't see how your patch may not regress
>>> some cases where the line wasn't output as an optimization?
>>
>> The new flag on block is kind of clearing that the BB is artificial and
>> in fact does not
>> belong to the line. I didn't want to do a bigger refactoring how blocks
>> are iterated via lines.
>>
>> Can you be please more specific about such a case?
>
> in profile.c I see
>
> if (name_differs || line_differs)
> {
> if (!*offset)
> {
> *offset = gcov_write_tag (GCOV_TAG_LINES);
> gcov_write_unsigned (bb->index);
> name_differs = line_differs=true;
> }
>
> ...
>
> so if line_differs is false we might not output GCOV_TAG_LINES either
> because 1) optimization, less stuff output, 2) the block has no line.
> Looks like we can't really distinguish those.
Agree with that.
>
> Not sure how on the input side we end up associating a BB with
> a line if nothing was output for it though.
>
> That is, with your change don't we need
>
> Index: gcc/profile.c
>
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for review, > Skimming over the patch I noticed you duplicate libiberties xcrc32 > functionality. should i take care about standalone libbacktrace ? https://github.com/ianlancetaylor/libbacktrace > Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature > is dwarf + elf specific but proper abstraction / #ifdefing should > ensure compiling > also succeeds for non-dwarf / non-elf platforms). thanks, i will move code to elf.c since it's about reading section from elf format. On 03/14/2017 11:27 AM, Richard Biener wrote: On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov wrote: Hello everyone, i have a patch for this issue. Great! List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo Skimming over the patch I noticed you duplicate libiberties xcrc32 functionality. Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature is dwarf + elf specific but proper abstraction / #ifdefing should ensure compiling also succeeds for non-dwarf / non-elf platforms). Leaving actual review to the maintainer (CCed). Thanks, Richard.
[PATCH 4/N] Do not ICE on an invalid input for MV.
Hello.
This fixes ICE when one does not provide valid target names:
__attribute__((target_clones("default,foo,bar")))
In that situation I suggest to print:
$ ./xgcc -B. /tmp/mvc-ice.c
/tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
foo ()
^~~
/tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
/tmp/mvc-ice.c: In function ‘foo.resolver’:
cc1: fatal error: At least one more version other than the default is expected
Should I add a test-case for a fatal error?
Thanks,
Martin
>From b00d0845869e82e7607fad692f278083520d6b47 Mon Sep 17 00:00:00 2001
From: marxin
Date: Tue, 14 Mar 2017 11:16:51 +0100
Subject: [PATCH] Do not ICE on an invalid input for MV.
gcc/ChangeLog:
2017-03-14 Martin Liska
* config/i386/i386.c (dispatch_function_versions): Display fatal
error instead of ICE.
---
gcc/config/i386/i386.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5fcd51f1385..ded9f14d02c 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -32670,7 +32670,10 @@ dispatch_function_versions (tree dispatch_decl,
/* At least one more version other than the default. */
num_versions = fndecls->length ();
- gcc_assert (num_versions >= 2);
+ if (num_versions < 2)
+fatal_error (UNKNOWN_LOCATION,
+ "At least one more version other than the default is "
+ "expected");
function_version_info = (struct _function_version_info *)
XNEWVEC (struct _function_version_info, (num_versions - 1));
--
2.11.1
Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).
On Tue, Mar 14, 2017 at 11:09 AM, Martin Liška wrote: > On 03/14/2017 10:46 AM, Richard Biener wrote: >> On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška wrote: >>> Hello. >>> >>> This is a small follow-up patch, where local.local flag should be also >>> dropped >>> for a default implementation. >>> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>> >>> Ready to be installed? >> >> I see we have clered the flag on the clones this way. But isn't it >> bogus to have >> it set in the first place? That is, isn't analysis sofar given bogus info? > > Yes, I did it for clones. Reason why we mark it is that local flag is set > by pass_ipa_function_and_variable_visibility pass, which runs before MV pass. > > I can imagine MV can bail out for a non-trivial reason and the visibility pass > should somehow simulate and predict what happens in the MV pass? Setting .local to true is an optimization, isn't it? So just not doing it when the attribute is present should be safe. Just "undoing" .local = true later is asking for trouble if some intermediate pass decides to do some transform based on .local = true, no? Richard. > Martin > >> >> Shouldn't we instead fix local_p to not mark functions with MV attribute >> local >> in the first place? >> >> Richard. >> >>> Martin >
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 11:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 10:12 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/14/2017 09:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 04:16 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:53 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:01 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Richard Biener wrote:
> >>>
> On Fri, 10 Mar 2017, Martin Liška wrote:
>
> > Hello.
> >
> > As briefly discussed in the PR, there are BB that do not
> > correspond to a real
> > line in source
> > code. profile.c emits locations for all BBs that have a gimple
> > statement
> > belonging to a line.
> > I hope these should be marked in gcov utility and not added in
> > --all-block
> > mode to counts of lines.
> >
> > Patch survives make check RUNTESTFLAGS="gcov.exp".
> >
> > Thanks for review and feedback.
>
> Humm, the patch doesn't seem to change the BBs associated with a
> line
> but rather somehow changes how we compute counts (the patch
> lacks a
> description of how you arrived at it). I expected the line-to-BB
> assignment process to be changed (whereever that is...).
> >>
> >> Currently, each basic block must belong to a source line. It's how
> >> gcov
> >> iterates all blocks (via lines).
> >>
> >>>
> >>> Ah, ok, looking at where output_location is called on I see we do
> >>> not
> >>> assign any line to that block. But still why does
> >>> line->has_block (arc->src) return true?
> >>
> >> Good objection! Problematic that 4->5 edge really comes from the
> >> same line.
> >>
> >>[0.00%]:
> >> ret_7 = 111;
> >> PROF_edge_counter_10 = __gcov0.UuT[0];
> >> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> >> __gcov0.UuT[0] = PROF_edge_counter_11;
> >>
> >>[0.00%]:
> >> # ret_1 = PHI
> >> goto ; [0.00%]
> >
> > Yes, but that's basically meaningless, unless not all edges come
> > from the
> > same line? I see nowhere where we'd explicitely assign lines to
> > edges so it must be gcov "reconstructing" this somewhere.
>
> That's why I added the another flag. We stream locations for basic
> blocks via
> 'output_location' function. And assignment blocks to lines happens
> here:
>
> static void
> add_line_counts (coverage_t *coverage, function_t *fn)
> {
> ...
> if (!ix || ix + 1 == fn->num_blocks)
> /* Entry or exit block */;
> else if (flag_all_blocks)
> {
> line_t *block_line = line;
>
> if (!block_line)
> block_line = &sources[fn->src].lines[fn->line];
>
> block->chain = block_line->u.blocks;
> block_line->u.blocks = block;
> }
>
> where line is always changes when we reach a BB that has a source
> line assigned. Thus it's changed
> for BB 4 and that's why BB 5 is added to same line.
> >>>
> >>> Ah, so this means we should "clear" the current line for BB 5 in
> >>> output_location? At least I don't see how your patch may not regress
> >>> some cases where the line wasn't output as an optimization?
> >>
> >> The new flag on block is kind of clearing that the BB is artificial
> >> and in fact does not
> >> belong to the line. I didn't want to do a bigger refactoring how
> >> blocks are iterated via lines.
> >>
> >> Can you be please more specific about such a case?
> >
> > in profile.c I see
> >
> > if (name_differs || line_differs)
> > {
> > if (!*offset)
> > {
> > *offset = gcov_write_tag (GCOV_TAG_LINES);
> > gcov_write_unsigned (bb->index);
> > name_differs = line_differs=true;
> > }
> >
> > ...
> >
> > so if line_differs is false we might not output GCOV_TAG_LINES either
> > because 1) optimization, less stuff output, 2) the block has no line.
> > Looks like we can't really distinguish those.
>
> Agree
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 11:30 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 11:13 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 10:12 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 09:13 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 04:16 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:53 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:01 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Richard Biener wrote:
>
>> On Fri, 10 Mar 2017, Martin Liška wrote:
>>
>>> Hello.
>>>
>>> As briefly discussed in the PR, there are BB that do not
>>> correspond to a real
>>> line in source
>>> code. profile.c emits locations for all BBs that have a gimple
>>> statement
>>> belonging to a line.
>>> I hope these should be marked in gcov utility and not added in
>>> --all-block
>>> mode to counts of lines.
>>>
>>> Patch survives make check RUNTESTFLAGS="gcov.exp".
>>>
>>> Thanks for review and feedback.
>>
>> Humm, the patch doesn't seem to change the BBs associated with a
>> line
>> but rather somehow changes how we compute counts (the patch
>> lacks a
>> description of how you arrived at it). I expected the line-to-BB
>> assignment process to be changed (whereever that is...).
Currently, each basic block must belong to a source line. It's how
gcov
iterates all blocks (via lines).
>
> Ah, ok, looking at where output_location is called on I see we do
> not
> assign any line to that block. But still why does
> line->has_block (arc->src) return true?
Good objection! Problematic that 4->5 edge really comes from the
same line.
[0.00%]:
ret_7 = 111;
PROF_edge_counter_10 = __gcov0.UuT[0];
PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
__gcov0.UuT[0] = PROF_edge_counter_11;
[0.00%]:
# ret_1 = PHI
goto ; [0.00%]
>>>
>>> Yes, but that's basically meaningless, unless not all edges come
>>> from the
>>> same line? I see nowhere where we'd explicitely assign lines to
>>> edges so it must be gcov "reconstructing" this somewhere.
>>
>> That's why I added the another flag. We stream locations for basic
>> blocks via
>> 'output_location' function. And assignment blocks to lines happens
>> here:
>>
>> static void
>> add_line_counts (coverage_t *coverage, function_t *fn)
>> {
>> ...
>> if (!ix || ix + 1 == fn->num_blocks)
>> /* Entry or exit block */;
>> else if (flag_all_blocks)
>> {
>>line_t *block_line = line;
>>
>>if (!block_line)
>> block_line = &sources[fn->src].lines[fn->line];
>>
>>block->chain = block_line->u.blocks;
>>block_line->u.blocks = block;
>> }
>>
>> where line is always changes when we reach a BB that has a source
>> line assigned. Thus it's changed
>> for BB 4 and that's why BB 5 is added to same line.
>
> Ah, so this means we should "clear" the current line for BB 5 in
> output_location? At least I don't see how your patch may not regress
> some cases where the line wasn't output as an optimization?
The new flag on block is kind of clearing that the BB is artificial
and in fact does not
belong to the line. I didn't want to do a bigger refactoring how
blocks are iterated via lines.
Can you be please more specific about such a case?
>>>
>>> in profile.c I see
>>>
>>> if (name_differs || line_differs)
>>> {
>>> if (!*offset)
>>> {
>>> *offset = gcov_write_tag (GCOV_TAG_LINES);
>>> gcov_write_unsigned (bb->index);
>>> name_differs = line_differs=true;
>>> }
>>>
>>> ...
>>>
>>> so if line_differs is false we might not output GCOV_TAG_LINES either
>>> because 1) optimization, less stuff output, 2) the block has no line.
>>> Looks like we ca
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 11:30 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 11:13 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/14/2017 10:12 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 09:13 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 04:16 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:53 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:01 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Richard Biener wrote:
> >
> >> On Fri, 10 Mar 2017, Martin Liška wrote:
> >>
> >>> Hello.
> >>>
> >>> As briefly discussed in the PR, there are BB that do not
> >>> correspond to a real
> >>> line in source
> >>> code. profile.c emits locations for all BBs that have a
> >>> gimple statement
> >>> belonging to a line.
> >>> I hope these should be marked in gcov utility and not added
> >>> in --all-block
> >>> mode to counts of lines.
> >>>
> >>> Patch survives make check RUNTESTFLAGS="gcov.exp".
> >>>
> >>> Thanks for review and feedback.
> >>
> >> Humm, the patch doesn't seem to change the BBs associated with
> >> a line
> >> but rather somehow changes how we compute counts (the patch
> >> lacks a
> >> description of how you arrived at it). I expected the
> >> line-to-BB
> >> assignment process to be changed (whereever that is...).
>
> Currently, each basic block must belong to a source line. It's
> how gcov
> iterates all blocks (via lines).
>
> >
> > Ah, ok, looking at where output_location is called on I see we
> > do not
> > assign any line to that block. But still why does
> > line->has_block (arc->src) return true?
>
> Good objection! Problematic that 4->5 edge really comes from
> the same line.
>
> [0.00%]:
> ret_7 = 111;
> PROF_edge_counter_10 = __gcov0.UuT[0];
> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> __gcov0.UuT[0] = PROF_edge_counter_11;
>
> [0.00%]:
> # ret_1 = PHI
> goto ; [0.00%]
> >>>
> >>> Yes, but that's basically meaningless, unless not all edges come
> >>> from the
> >>> same line? I see nowhere where we'd explicitely assign lines to
> >>> edges so it must be gcov "reconstructing" this somewhere.
> >>
> >> That's why I added the another flag. We stream locations for basic
> >> blocks via
> >> 'output_location' function. And assignment blocks to lines happens
> >> here:
> >>
> >> static void
> >> add_line_counts (coverage_t *coverage, function_t *fn)
> >> {
> >> ...
> >> if (!ix || ix + 1 == fn->num_blocks)
> >>/* Entry or exit block */;
> >> else if (flag_all_blocks)
> >>{
> >> line_t *block_line = line;
> >>
> >> if (!block_line)
> >>block_line = &sources[fn->src].lines[fn->line];
> >>
> >> block->chain = block_line->u.blocks;
> >> block_line->u.blocks = block;
> >>}
> >>
> >> where line is always changes when we reach a BB that has a source
> >> line assigned. Thus it's changed
> >> for BB 4 and that's why BB 5 is added to same line.
> >
> > Ah, so this means we should "clear" the current line for BB 5 in
> > output_location? At least I don't see how your patch may not
> > regress
> > some cases where the line wasn't output as an optimization?
>
> The new flag on block is kind of clearing that the BB is artificial
> and in fact does not
> belong to the line. I didn't want to do a bigger refactoring how
> blocks are iterated via lines.
>
> Can you be please more specific about such a case?
> >>>
> >>> in profile.c I see
> >>>
> >>> if (name_differs || line_differs)
> >>> {
> >>> if (!*offset)
> >>> {
> >>> *offset = gcov_write_tag (GCOV_TAG_LINES);
> >>> gcov_write_unsig
Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).
On 03/14/2017 11:29 AM, Richard Biener wrote:
> On Tue, Mar 14, 2017 at 11:09 AM, Martin Liška wrote:
>> On 03/14/2017 10:46 AM, Richard Biener wrote:
>>> On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška wrote:
Hello.
This is a small follow-up patch, where local.local flag should be also
dropped
for a default implementation.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
>>>
>>> I see we have clered the flag on the clones this way. But isn't it
>>> bogus to have
>>> it set in the first place? That is, isn't analysis sofar given bogus info?
>>
>> Yes, I did it for clones. Reason why we mark it is that local flag is set
>> by pass_ipa_function_and_variable_visibility pass, which runs before MV pass.
>>
>> I can imagine MV can bail out for a non-trivial reason and the visibility
>> pass
>> should somehow simulate and predict what happens in the MV pass?
>
> Setting .local to true is an optimization, isn't it? So just not doing it
> when
> the attribute is present should be safe. Just "undoing" .local = true later
> is asking for trouble if some intermediate pass decides to do some transform
> based on .local = true, no?
Ok, there's more detail analysis. When a MV function is set local (either it's
a static symbol),
or IPA split (or IPA inline) makes a clone. This is all fine because we define
local as:
struct GTY(()) cgraph_local_info {
/* Set when function is visible in current compilation unit only and
its address is never taken. */
unsigned local : 1;
All IPA passes can happily makes optimization based on that. However, in moment
where we
introduce a dispatcher (that obviously takes an address), then the function
starts to not
be local.
Thus I believe my original patch (and the part which is already in trunk)
should work
in the right way.
Martin
>
> Richard.
>
>> Martin
>>
>>>
>>> Shouldn't we instead fix local_p to not mark functions with MV attribute
>>> local
>>> in the first place?
>>>
>>> Richard.
>>>
Martin
>>
Re: [PATCH 3/3] Verify that target can create a dispatcher call (PR target/79892).
On Mon, Mar 13, 2017 at 9:26 AM, marxin wrote:
> gcc/ChangeLog:
Ok.
Richard.
> 2017-03-13 Martin Liska
>
> * multiple_target.c (create_dispatcher_calls): Check that
> a target can create a function dispatcher.
> ---
> gcc/multiple_target.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
> index 7b735ae81ae..cb792262d6e 100644
> --- a/gcc/multiple_target.c
> +++ b/gcc/multiple_target.c
> @@ -68,6 +68,13 @@ create_dispatcher_calls (struct cgraph_node *node)
> " supported by this target");
> break;
> }
> + else if (!targetm.get_function_versions_dispatcher)
> + {
> + error_at (gimple_location (call),
> + "target does not support function version dispatcher");
> + break;
> + }
> +
>e_next = e->next_caller;
>idecl = targetm.get_function_versions_dispatcher (decl);
>if (!idecl)
> --
> 2.11.1
>
Re: [PATCH 4/N] Do not ICE on an invalid input for MV.
On Tue, Mar 14, 2017 at 11:24 AM, Martin Liška wrote:
> Hello.
>
> This fixes ICE when one does not provide valid target names:
> __attribute__((target_clones("default,foo,bar")))
>
> In that situation I suggest to print:
>
> $ ./xgcc -B. /tmp/mvc-ice.c
> /tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
> foo ()
> ^~~
> /tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
> /tmp/mvc-ice.c: In function ‘foo.resolver’:
> cc1: fatal error: At least one more version other than the default is expected
>
> Should I add a test-case for a fatal error?
Better avoid fatal_error, can't you just return early after regular error()?
Do we error similarly for __attribute__((target_clones("default")))
from generic code?
I wonder if we can just go ahead even with num_versions == 1?
>
> Thanks,
> Martin
Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).
On Tue, Mar 14, 2017 at 12:04 PM, Martin Liška wrote:
> On 03/14/2017 11:29 AM, Richard Biener wrote:
>> On Tue, Mar 14, 2017 at 11:09 AM, Martin Liška wrote:
>>> On 03/14/2017 10:46 AM, Richard Biener wrote:
On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška wrote:
> Hello.
>
> This is a small follow-up patch, where local.local flag should be also
> dropped
> for a default implementation.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
I see we have clered the flag on the clones this way. But isn't it
bogus to have
it set in the first place? That is, isn't analysis sofar given bogus info?
>>>
>>> Yes, I did it for clones. Reason why we mark it is that local flag is set
>>> by pass_ipa_function_and_variable_visibility pass, which runs before MV
>>> pass.
>>>
>>> I can imagine MV can bail out for a non-trivial reason and the visibility
>>> pass
>>> should somehow simulate and predict what happens in the MV pass?
>>
>> Setting .local to true is an optimization, isn't it? So just not doing it
>> when
>> the attribute is present should be safe. Just "undoing" .local = true later
>> is asking for trouble if some intermediate pass decides to do some transform
>> based on .local = true, no?
>
> Ok, there's more detail analysis. When a MV function is set local (either
> it's a static symbol),
> or IPA split (or IPA inline) makes a clone. This is all fine because we
> define local as:
>
> struct GTY(()) cgraph_local_info {
> /* Set when function is visible in current compilation unit only and
> its address is never taken. */
> unsigned local : 1;
>
> All IPA passes can happily makes optimization based on that. However, in
> moment where we
> introduce a dispatcher (that obviously takes an address), then the function
> starts to not
> be local.
>
> Thus I believe my original patch (and the part which is already in trunk)
> should work
> in the right way.
Ah, ok. I thought it was also supposed to preserve the ABI but if
only the ABI of the
dispatcher (the original fndecl) matters then this is moot.
Patch is ok then.
Thanks,
Richard.
> Martin
>
>>
>> Richard.
>>
>>> Martin
>>>
Shouldn't we instead fix local_p to not mark functions with MV attribute
local
in the first place?
Richard.
> Martin
>>>
>
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 11:48 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 11:30 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 11:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 10:12 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 09:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 04:16 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:53 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:01 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Richard Biener wrote:
>>>
On Fri, 10 Mar 2017, Martin Liška wrote:
> Hello.
>
> As briefly discussed in the PR, there are BB that do not
> correspond to a real
> line in source
> code. profile.c emits locations for all BBs that have a
> gimple statement
> belonging to a line.
> I hope these should be marked in gcov utility and not added
> in --all-block
> mode to counts of lines.
>
> Patch survives make check RUNTESTFLAGS="gcov.exp".
>
> Thanks for review and feedback.
Humm, the patch doesn't seem to change the BBs associated with
a line
but rather somehow changes how we compute counts (the patch
lacks a
description of how you arrived at it). I expected the
line-to-BB
assignment process to be changed (whereever that is...).
>>
>> Currently, each basic block must belong to a source line. It's
>> how gcov
>> iterates all blocks (via lines).
>>
>>>
>>> Ah, ok, looking at where output_location is called on I see we
>>> do not
>>> assign any line to that block. But still why does
>>> line->has_block (arc->src) return true?
>>
>> Good objection! Problematic that 4->5 edge really comes from
>> the same line.
>>
>>[0.00%]:
>> ret_7 = 111;
>> PROF_edge_counter_10 = __gcov0.UuT[0];
>> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
>> __gcov0.UuT[0] = PROF_edge_counter_11;
>>
>>[0.00%]:
>> # ret_1 = PHI
>> goto ; [0.00%]
>
> Yes, but that's basically meaningless, unless not all edges come
> from the
> same line? I see nowhere where we'd explicitely assign lines to
> edges so it must be gcov "reconstructing" this somewhere.
That's why I added the another flag. We stream locations for basic
blocks via
'output_location' function. And assignment blocks to lines happens
here:
static void
add_line_counts (coverage_t *coverage, function_t *fn)
{
...
if (!ix || ix + 1 == fn->num_blocks)
/* Entry or exit block */;
else if (flag_all_blocks)
{
line_t *block_line = line;
if (!block_line)
block_line = &sources[fn->src].lines[fn->line];
block->chain = block_line->u.blocks;
block_line->u.blocks = block;
}
where line is always changes when we reach a BB that has a source
line assigned. Thus it's changed
for BB 4 and that's why BB 5 is added to same line.
>>>
>>> Ah, so this means we should "clear" the current line for BB 5 in
>>> output_location? At least I don't see how your patch may not
>>> regress
>>> some cases where the line wasn't output as an optimization?
>>
>> The new flag on block is kind of clearing that the BB is artificial
>> and in fact does not
>> belong to the line. I didn't want to do a bigger refactoring how
>> blocks are iterated via lines.
>>
>> Can you be please more specific about such a case?
>
> in profile.c I see
>
> if (name_differs || line_differs)
> {
> if (!*offset)
> {
> *offset = gcov_write_tag (GCO
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 12:55 PM, Martin Liška wrote:
> On 03/14/2017 11:48 AM, Richard Biener wrote:
>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>
>>> On 03/14/2017 11:30 AM, Richard Biener wrote:
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 11:13 AM, Richard Biener wrote:
>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>
>>> On 03/14/2017 10:12 AM, Richard Biener wrote:
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 09:13 AM, Richard Biener wrote:
>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>
>>> On 03/13/2017 04:16 PM, Richard Biener wrote:
On Mon, 13 Mar 2017, Martin Liška wrote:
> On 03/13/2017 02:53 PM, Richard Biener wrote:
>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>
>>> On 03/13/2017 02:01 PM, Richard Biener wrote:
On Mon, 13 Mar 2017, Richard Biener wrote:
> On Fri, 10 Mar 2017, Martin Liška wrote:
>
>> Hello.
>>
>> As briefly discussed in the PR, there are BB that do not
>> correspond to a real
>> line in source
>> code. profile.c emits locations for all BBs that have a
>> gimple statement
>> belonging to a line.
>> I hope these should be marked in gcov utility and not added
>> in --all-block
>> mode to counts of lines.
>>
>> Patch survives make check RUNTESTFLAGS="gcov.exp".
>>
>> Thanks for review and feedback.
>
> Humm, the patch doesn't seem to change the BBs associated
> with a line
> but rather somehow changes how we compute counts (the patch
> lacks a
> description of how you arrived at it). I expected the
> line-to-BB
> assignment process to be changed (whereever that is...).
>>>
>>> Currently, each basic block must belong to a source line. It's
>>> how gcov
>>> iterates all blocks (via lines).
>>>
Ah, ok, looking at where output_location is called on I see we
do not
assign any line to that block. But still why does
line->has_block (arc->src) return true?
>>>
>>> Good objection! Problematic that 4->5 edge really comes from
>>> the same line.
>>>
>>>[0.00%]:
>>> ret_7 = 111;
>>> PROF_edge_counter_10 = __gcov0.UuT[0];
>>> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
>>> __gcov0.UuT[0] = PROF_edge_counter_11;
>>>
>>>[0.00%]:
>>> # ret_1 = PHI
>>> goto ; [0.00%]
>>
>> Yes, but that's basically meaningless, unless not all edges come
>> from the
>> same line? I see nowhere where we'd explicitely assign lines to
>> edges so it must be gcov "reconstructing" this somewhere.
>
> That's why I added the another flag. We stream locations for
> basic blocks via
> 'output_location' function. And assignment blocks to lines
> happens here:
>
> static void
> add_line_counts (coverage_t *coverage, function_t *fn)
> {
> ...
> if (!ix || ix + 1 == fn->num_blocks)
> /* Entry or exit block */;
> else if (flag_all_blocks)
> {
> line_t *block_line = line;
>
> if (!block_line)
> block_line = &sources[fn->src].lines[fn->line];
>
> block->chain = block_line->u.blocks;
> block_line->u.blocks = block;
> }
>
> where line is always changes when we reach a BB that has a source
> line assigned. Thus it's changed
> for BB 4 and that's why BB 5 is added to same line.
Ah, so this means we should "clear" the current line for BB 5 in
output_location? At least I don't see how your patch may not
regress
some cases where the line wasn't output as an optimization?
>>>
>>> The new flag on block is kind of clearing that the BB is artificial
>>> and in fact does not
>>> belong to the line. I didn't want to do a bigger refactoring how
>>> blocks are iterated via lines.
>>>
>>> Can you be please more specific about such a case?
>>
>> in profile.c I see
>>
>>>
Re: [PATCH 4/N] Do not ICE on an invalid input for MV.
On 03/14/2017 12:09 PM, Richard Biener wrote:
> On Tue, Mar 14, 2017 at 11:24 AM, Martin Liška wrote:
>> Hello.
>>
>> This fixes ICE when one does not provide valid target names:
>> __attribute__((target_clones("default,foo,bar")))
>>
>> In that situation I suggest to print:
>>
>> $ ./xgcc -B. /tmp/mvc-ice.c
>> /tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
>> foo ()
>> ^~~
>> /tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
>> /tmp/mvc-ice.c: In function ‘foo.resolver’:
>> cc1: fatal error: At least one more version other than the default is
>> expected
>>
>> Should I add a test-case for a fatal error?
>
> Better avoid fatal_error, can't you just return early after regular error()?
Yep. Early bail out would be better.
>
> Do we error similarly for __attribute__((target_clones("default")))
> from generic code?
>
> I wonder if we can just go ahead even with num_versions == 1?
It's covered in MV pass:
$ ./xgcc -B. /tmp/mvc-single.c
/tmp/mvc-single.c:6:1: warning: single target_clones attribute is ignored
foo ()
Martin
>
>
>>
>> Thanks,
>> Martin
>From 5e7aa55549fabeb4c7fc4b8339dba210c49614d0 Mon Sep 17 00:00:00 2001
From: marxin
Date: Tue, 14 Mar 2017 11:16:51 +0100
Subject: [PATCH] Do not ICE on an invalid input for MV.
gcc/ChangeLog:
2017-03-14 Martin Liska
* multiple_target.c (expand_target_clones): Bail out for
an invalid attribute.
---
gcc/multiple_target.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
index 7b735ae81ae..1e02116a2db 100644
--- a/gcc/multiple_target.c
+++ b/gcc/multiple_target.c
@@ -296,10 +296,7 @@ expand_target_clones (struct cgraph_node *node, bool definition)
if (!targetm.target_option.valid_attribute_p (new_node->decl, NULL,
TREE_VALUE (attributes),
0))
- {
- input_location = saved_loc;
- continue;
- }
+ return false;
input_location = saved_loc;
decl2_v = new_node->function_version ();
--
2.11.1
[PATCH] [PR79542][Ada] Fix ICE in dwarf2out.c with nested func. inlining
Hello,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79542 reports an ICE in
dwarf2out.c for an Ada testcase built with optimization.
This crash happens during the late generation pass because
add_gnat_descriptive_type cannot find the type DIE corresponding to some
descriptive type after having tried to generate it. This is because the
DIE was generated during the early generation pass, but then pruned by
the type pruning machinery. So why was it pruned?
We are in a situation where we have cloned types (because of inlining,
IIUC) whose TYPE_NAME have non-null DECL_ABSTRACT_ORIGIN attributes. As
a consequence:
* In modified_type_die, the "handle C typedef types" part calls
gen_type_die on the cloned type.
* gen_type_die matches a typedef variant, and then calls gen_decl_die
on its TYPE_NAME, which will end up calling gen_typedef_die.
* gen_typedef_die checks decl_ultimate_origin for this TYPE_DECL, and
finds one, so it only adds a DW_AT_abstract_origin attribute to the
DW_TAG_typedef DIE, but the cloned type itself does not get its own
DIE.
* Back in modified_type_die, the call to lookup_type_die on the type
passed to gen_type_die returns NULL.
In the end, whole type trees, i.e. the ones referenced by
DECL_ABSTRACT_ORIGIN attributes, are never referenced from type pruning
"roots" and are thus pruned. The descriptive type at stake here is one
of them, hence the assertion failure.
This patch attemps to fix that with what seems to be the most sensible
thing to do in my opinion: updating the "handle C typedef types" part in
modified_type_die to check decl_ultimate_origin before calling
gen_type_die: if that function returns something not null, then we know
that gen_type_die/gen_typedef_die will not generate a DIE for the input
type, so we try to process the ultimate origin instead.
I bootstrapped and regtested this successfully on x86_64-linux. I also
checked that there was no regression in GDB's testsuite and of course
the new testcase, which crashes with current trunk, passes with the
patch applied.
Is it ok to commit? Thank you in advance!
gcc/
PR ada/79542
* dwarf2out.c (modified_type_die): For C typedef types that have
an ultimate origin, process the ultimate origin instead of the
input type.
gcc/testsuite/
* gnat.dg/debug10.ads, gnat.dg/debug10.adb: New testcase.
---
gcc/dwarf2out.c | 12
gcc/testsuite/gnat.dg/debug10.adb | 38 ++
gcc/testsuite/gnat.dg/debug10.ads | 5 +
3 files changed, 55 insertions(+)
create mode 100644 gcc/testsuite/gnat.dg/debug10.adb
create mode 100644 gcc/testsuite/gnat.dg/debug10.ads
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 0bbb90ed3aa..4088614fc18 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -12496,6 +12496,18 @@ modified_type_die (tree type, int cv_quals, bool
reverse,
if (qualified_type == dtype)
{
+ tree origin
+ = TYPE_NAME (qualified_type) == NULL
+? NULL
+: decl_ultimate_origin (TYPE_NAME (qualified_type));
+
+ /* Typedef variants that have an abstract origin don't get their own
+type DIE (see gen_typedef_die), so fall back on the ultimate
+abstract origin instead. */
+ if (origin != NULL)
+ return modified_type_die (TREE_TYPE (origin), cv_quals, reverse,
+ context_die);
+
/* For a named type, use the typedef. */
gen_type_die (qualified_type, context_die);
return lookup_type_die (qualified_type);
diff --git a/gcc/testsuite/gnat.dg/debug10.adb
b/gcc/testsuite/gnat.dg/debug10.adb
new file mode 100644
index 000..b85f3d36dd7
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/debug10.adb
@@ -0,0 +1,38 @@
+-- { dg-options "-cargs -O2 -g -margs" }
+
+package body Debug10 is
+
+ procedure Compile (P : Natural)
+ is
+ Max_Pos : constant Natural := P;
+ type Position_Set is array (1 .. Max_Pos) of Boolean;
+
+ Empty : constant Position_Set := (others => False);
+
+ type Position_Set_Array is array (1 .. Max_Pos) of Position_Set;
+
+ Follow : Position_Set_Array := (others => Empty);
+
+ function Get_Follows return Position_Set;
+
+ procedure Make_DFA;
+
+ function Get_Follows return Position_Set is
+ Result : Position_Set := Empty;
+ begin
+ Result := Result or Follow (1);
+
+ return Result;
+ end Get_Follows;
+
+ procedure Make_DFA is
+ Next : constant Position_Set := Get_Follows;
+ begin
+ null;
+ end Make_DFA;
+
+ begin
+ Make_DFA;
+ end Compile;
+
+end Debug10;
diff --git a/gcc/testsuite/gnat.dg/debug10.ads
b/gcc/testsuite/gnat.dg/debug10.ads
new file mode 100644
index 000..ecd3c8880ba
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/debug10.ads
@@ -0,0 +1,5 @@
+package Debug10 is
+
+ procedure Compile (
GCC 7.1 Status report (2017-03-14)
Status == The trunk is in regression and documentation fixes mode (Stage 4) thus as if it were a release branch. We are feature complete since quite a while and just chasing down P1 bugs which block the release of GCC 7. Tentative release date is mid April which means, given past history, a branching target of early April. That leaves us about three weeks for chasing down the remaining P1 regressions. Please help with this task. Quality Data Priority # Change from last report --- --- P1 17 - 8 P2 108 - 19 P3 19 - 18 P4 138 + 4 P5 30 --- --- Total P1-P3 144 - 45 Total 312 - 41 Previous Report === https://gcc.gnu.org/ml/gcc/2017-01/msg00181.html
[PATCH] Install gcov-dump.
Tested on my local machine that's properly installed.
Ready for trunk?
Thanks,
Martin
>From 553c8402f8b0c77a30f4476918de1e4acff34dbc Mon Sep 17 00:00:00 2001
From: marxin
Date: Tue, 14 Mar 2017 13:26:49 +0100
Subject: [PATCH] Install gcov-dump.
gcc/ChangeLog:
2017-03-14 Martin Liska
* Makefile.in: Install gcov-dump.
---
gcc/Makefile.in | 10 ++
1 file changed, 10 insertions(+)
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 8648d681fec..1ba0475c893 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -806,6 +806,7 @@ GCC_TARGET_INSTALL_NAME := $(target_noncanonical)-$(shell echo gcc|sed '$(progra
CPP_INSTALL_NAME := $(shell echo cpp|sed '$(program_transform_name)')
GCOV_INSTALL_NAME := $(shell echo gcov|sed '$(program_transform_name)')
GCOV_TOOL_INSTALL_NAME := $(shell echo gcov-tool|sed '$(program_transform_name)')
+GCOV_DUMP_INSTALL_NAME := $(shell echo gcov-dump|sed '$(program_transform_name)')
# Setup the testing framework, if you have one
EXPECT = `if [ -f $${rootme}/../expect/expect ] ; then \
@@ -3519,6 +3520,15 @@ install-common: native lang.install-common installdirs
gcov-tool$(exeext) $(DESTDIR)$(bindir)/$(GCOV_TOOL_INSTALL_NAME)$(exeext); \
fi; \
fi
+# Install gcov-dump if it was compiled.
+ -if test "$(enable_as_accelerator)" != "yes" ; then \
+ if [ -f gcov-dump$(exeext) ]; \
+ then \
+ rm -f $(DESTDIR)$(bindir)/$(GCOV_DUMP_INSTALL_NAME)$(exeext); \
+ $(INSTALL_PROGRAM) \
+ gcov-dump$(exeext) $(DESTDIR)$(bindir)/$(GCOV_DUMP_INSTALL_NAME)$(exeext); \
+ fi; \
+ fi
# Install the driver program as $(target_noncanonical)-gcc,
# $(target_noncanonical)-gcc-$(version), and also as gcc if native.
--
2.11.1
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 11:48 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 11:30 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/14/2017 11:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 10:12 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/14/2017 09:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 04:16 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:53 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:01 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Richard Biener wrote:
> >>>
> On Fri, 10 Mar 2017, Martin Liška wrote:
>
> > Hello.
> >
> > As briefly discussed in the PR, there are BB that do not
> > correspond to a real
> > line in source
> > code. profile.c emits locations for all BBs that have a
> > gimple statement
> > belonging to a line.
> > I hope these should be marked in gcov utility and not added
> > in --all-block
> > mode to counts of lines.
> >
> > Patch survives make check RUNTESTFLAGS="gcov.exp".
> >
> > Thanks for review and feedback.
>
> Humm, the patch doesn't seem to change the BBs associated
> with a line
> but rather somehow changes how we compute counts (the patch
> lacks a
> description of how you arrived at it). I expected the
> line-to-BB
> assignment process to be changed (whereever that is...).
> >>
> >> Currently, each basic block must belong to a source line. It's
> >> how gcov
> >> iterates all blocks (via lines).
> >>
> >>>
> >>> Ah, ok, looking at where output_location is called on I see
> >>> we do not
> >>> assign any line to that block. But still why does
> >>> line->has_block (arc->src) return true?
> >>
> >> Good objection! Problematic that 4->5 edge really comes from
> >> the same line.
> >>
> >>[0.00%]:
> >> ret_7 = 111;
> >> PROF_edge_counter_10 = __gcov0.UuT[0];
> >> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> >> __gcov0.UuT[0] = PROF_edge_counter_11;
> >>
> >>[0.00%]:
> >> # ret_1 = PHI
> >> goto ; [0.00%]
> >
> > Yes, but that's basically meaningless, unless not all edges
> > come from the
> > same line? I see nowhere where we'd explicitely assign lines to
> > edges so it must be gcov "reconstructing" this somewhere.
>
> That's why I added the another flag. We stream locations for
> basic blocks via
> 'output_location' function. And assignment blocks to lines
> happens here:
>
> static void
> add_line_counts (coverage_t *coverage, function_t *fn)
> {
> ...
> if (!ix || ix + 1 == fn->num_blocks)
> /* Entry or exit block */;
> else if (flag_all_blocks)
> {
> line_t *block_line = line;
>
> if (!block_line)
> block_line = &sources[fn->src].lines[fn->line];
>
> block->chain = block_line->u.blocks;
> block_line->u.blocks = block;
> }
>
> where line is always changes when we reach a BB that has a
> source line assigned. Thus it's changed
> for BB 4 and that's why BB 5 is added to same line.
> >>>
> >>> Ah, so this means we should "clear" the current line for BB 5 in
> >>> output_location? At least I don't see how your patch may not
> >>> regress
> >>> some cases where the line wasn't output as an optimization?
> >>
> >> The new flag on block is kind of clearing that the BB is
> >> artificial and in fact does not
> >> belong to the line. I didn't want to do a bigger refactoring how
> >> blocks are iterated via lines.
> >>
> >> Can y
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> > /tmp/gcov-1.gcno: block
> > 2:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':10,
> > 14
> > /tmp/gcov-1.gcno: block
> > 4:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':14
> >
> > where blocks 2 and 4 are:
> >
> >[0.00%]:
> > i_3 = 0;
> > goto ; [0.00%]
> >
> > ...
> >
> >[0.00%]:
> > # i_1 = PHI
> > if (i_1 <= 9)
> > goto ; [0.00%]
> > else
> > goto ; [0.00%]
> >
> > The same happens to int a = b < 1 ? (c < 3 ? d : c) : a;
> >
> > /tmp/gcov2.gcno:block 2:`/tmp/gcov2.c':1, 3
> >
> >[0.00%]:
> > if (b_3(D) <= 0)
> > goto ; [0.00%]
> > else
> > goto ; [0.00%]
> >
> > That showed a caching of locations actually magically handles loops and
> > ternary operations.
> > I'm still wondering how should be defined line count for a multiple
> > statements happening
> > on the line? Having that we can find a proper solution.
>
> It should be number of times the line is _entered_, that is, lineno
> changed from something != lineno to lineno. Consider
>
> foo (); goto baz; lab: bar (); // line 1
> baz:
> goto lab;
>
> should increment line 1 when entering to foo () as well as when
> entering through goto lab. but both times just once.
Of course with -a you are supposed to get sub-line accuracy for these
cases, but then you'll get not a single number per line (and not sure
how you can reasonably interpret -a info without column info or so).
Richard.
> Richard.
>
>
> > Martin
> >
> > >
> > > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > > {
> > > gimple *stmt = gsi_stmt (gsi);
> > > if (!RESERVED_LOCATION_P (gimple_location (stmt)))
> > > output_location (gimple_filename (stmt), gimple_lineno
> > > (stmt),
> > > &offset, bb);
> > >
> > > should use expand_location and then look at the spelling location,
> > > otherwise we'll get interesting effects with macro expansion?
> > >
> > > }
> > >
> > > Richard.
> > >
> > >> Martin
> > >>
> > >>
> > >>>
> > >>> Richard.
> > >>>
> > Martin
> >
> > >
> > >> Martin
> > >>
> > >>>
> > >>>
> > Martin
> >
> > >
> > > Richard.
> > >
> > >
> > >> Hope Nathan will find time to provide review as he's familiar
> > >> with content of gcov.c.
> > >>
> > >> Martin
> > >>
> > >>>
> > >>> OTOH I don't know much about gcov format.
> > >>>
> > >>> Richard.
> > >>>
> > Martin
> >
> > >
> > > Richard.
> > >
> > >> Martin
> > >>
> > >>>
> > >>> Richard.
> > >>>
> > >>
> > >>
> > >
> >
> >
> > >>>
> > >>
> > >>
> > >
> >
> >
> > >>>
> > >>
> > >>
> > >
> >
> >
> > >>>
> > >>
> > >>
> > >
> >
> >
>
>
--
Richard Biener
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 01:33 PM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 11:48 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 11:30 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 11:13 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 10:12 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 09:13 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 04:16 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:53 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:01 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Richard Biener wrote:
>
>> On Fri, 10 Mar 2017, Martin Liška wrote:
>>
>>> Hello.
>>>
>>> As briefly discussed in the PR, there are BB that do not
>>> correspond to a real
>>> line in source
>>> code. profile.c emits locations for all BBs that have a
>>> gimple statement
>>> belonging to a line.
>>> I hope these should be marked in gcov utility and not added
>>> in --all-block
>>> mode to counts of lines.
>>>
>>> Patch survives make check RUNTESTFLAGS="gcov.exp".
>>>
>>> Thanks for review and feedback.
>>
>> Humm, the patch doesn't seem to change the BBs associated
>> with a line
>> but rather somehow changes how we compute counts (the patch
>> lacks a
>> description of how you arrived at it). I expected the
>> line-to-BB
>> assignment process to be changed (whereever that is...).
Currently, each basic block must belong to a source line. It's
how gcov
iterates all blocks (via lines).
>
> Ah, ok, looking at where output_location is called on I see
> we do not
> assign any line to that block. But still why does
> line->has_block (arc->src) return true?
Good objection! Problematic that 4->5 edge really comes from
the same line.
[0.00%]:
ret_7 = 111;
PROF_edge_counter_10 = __gcov0.UuT[0];
PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
__gcov0.UuT[0] = PROF_edge_counter_11;
[0.00%]:
# ret_1 = PHI
goto ; [0.00%]
>>>
>>> Yes, but that's basically meaningless, unless not all edges
>>> come from the
>>> same line? I see nowhere where we'd explicitely assign lines to
>>> edges so it must be gcov "reconstructing" this somewhere.
>>
>> That's why I added the another flag. We stream locations for
>> basic blocks via
>> 'output_location' function. And assignment blocks to lines
>> happens here:
>>
>> static void
>> add_line_counts (coverage_t *coverage, function_t *fn)
>> {
>> ...
>> if (!ix || ix + 1 == fn->num_blocks)
>> /* Entry or exit block */;
>> else if (flag_all_blocks)
>> {
>>line_t *block_line = line;
>>
>>if (!block_line)
>> block_line = &sources[fn->src].lines[fn->line];
>>
>>block->chain = block_line->u.blocks;
>>block_line->u.blocks = block;
>> }
>>
>> where line is always changes when we reach a BB that has a
>> source line assigned. Thus it's changed
>> for BB 4 and that's why BB 5 is added to same line.
>
> Ah, so this means we should "clear" the current line for BB 5 in
> output_location? At least I don't see how your patch may not
> regress
> some cases where the line wasn't output as an optimization?
The new flag on block is kind of clearing that the BB is
artificial and in fact does not
belong to the line. I didn't want to do a bigger refactoring how
blocks are i
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 3:57 AM, Richard Biener > wrote: > > On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt > wrote: >> >> Index: gcc/tree-stdarg.c >> === >> --- gcc/tree-stdarg.c (revision 246109) >> +++ gcc/tree-stdarg.c (working copy) >> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun) >> types. */ >>gimplify_assign (lhs, expr, &pre); >> } >> - else >> + else if (is_gimple_addressable (expr)) >> gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > > This is wrong - we lose side-effects this way and the only reason we gimplify > is to _not_ lose them. > > Better is sth like > > Index: gcc/tree-stdarg.c > === > --- gcc/tree-stdarg.c (revision 246082) > +++ gcc/tree-stdarg.c (working copy) > @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) >gimplify_assign (lhs, expr, &pre); > } >else > - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); > >input_location = saved_location; >pop_gimplify_context (NULL); OK, thanks for the explanation. Unfortunately this fails bootstrap with a failed assert a little later. I'll dig further. Bill
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
On Tue, Mar 14, 2017 at 3:20 AM, Denis Khalikov wrote: > Thanks for review, > >> Skimming over the patch I noticed you duplicate libiberties xcrc32 >> functionality. > > should i take care about standalone libbacktrace ? > https://github.com/ianlancetaylor/libbacktrace No, don't worry about it. Ian
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Ok, thanks, i will change patch to use crc32 from libiberty and also implement searching for debuginfo with build id. As it was implemented to binutils PR binutils/20876. On 03/14/2017 04:21 PM, Ian Lance Taylor wrote: On Tue, Mar 14, 2017 at 3:20 AM, Denis Khalikov wrote: Thanks for review, Skimming over the patch I noticed you duplicate libiberties xcrc32 functionality. should i take care about standalone libbacktrace ? https://github.com/ianlancetaylor/libbacktrace No, don't worry about it. Ian
Re: [PATCH 4/N] Do not ICE on an invalid input for MV.
On Tue, Mar 14, 2017 at 1:24 PM, Martin Liška wrote:
> On 03/14/2017 12:09 PM, Richard Biener wrote:
>> On Tue, Mar 14, 2017 at 11:24 AM, Martin Liška wrote:
>>> Hello.
>>>
>>> This fixes ICE when one does not provide valid target names:
>>> __attribute__((target_clones("default,foo,bar")))
>>>
>>> In that situation I suggest to print:
>>>
>>> $ ./xgcc -B. /tmp/mvc-ice.c
>>> /tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
>>> foo ()
>>> ^~~
>>> /tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
>>> /tmp/mvc-ice.c: In function ‘foo.resolver’:
>>> cc1: fatal error: At least one more version other than the default is
>>> expected
>>>
>>> Should I add a test-case for a fatal error?
>>
>> Better avoid fatal_error, can't you just return early after regular error()?
>
> Yep. Early bail out would be better.
>
>>
>> Do we error similarly for __attribute__((target_clones("default")))
>> from generic code?
>>
>> I wonder if we can just go ahead even with num_versions == 1?
>
> It's covered in MV pass:
>
> $ ./xgcc -B. /tmp/mvc-single.c
> /tmp/mvc-single.c:6:1: warning: single target_clones attribute is ignored
> foo ()
I see.
Attached patch is ok.
Thanks,
Richard.
> Martin
>
>>
>>
>>>
>>> Thanks,
>>> Martin
>
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov wrote: > Hello everyone, i have a patch for this issue. > > List of implemented functionality: > > 1.Reading .gnu_debuglink section from ELF file: > a. Reading name of debug info file. > b. Verifying crc32 sum. > > 2. Searching for separate debug info file from paths: > a. /usr/lib/debug/path/to/executable > b. /path/to/executable > c. /path/to/executable/.debug > > Assumed that debug info file generated by objcopy from binutils. > > objcopy --only-keep-debug foo foo.debug > strip -g foo > objcopy --add-gnu-debuglink=foo.debug foo > +clean_separate:glinktest.debug > + rm -f glinktest.debug > + > +separate: glinktest > + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ > + then \ > + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ > + $(STRIP) glinktest;\ > + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ > + fi; As far as I know "separate" is not a special thing in automake. We should always run (and clean) this test if the necessary objcopy option is available. > +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h Missing '$' in "$(INCDIR)". > +/* Return 1 if header is valid and -1 on fail */ This comment does not explain what the function actually does. > +/* Return the pointer to char array with data from .gnudebuglink section > inside. */ Line too long, we use 80 column lines. > +unsigned char * > +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor, > + backtrace_error_callback error_callback, void *data, > + int exe, int *gnulink_data_len_out) This should be static. I see that you are calling it elsewhere, but it doesn't make sense to call an "elf" function outside of elf.c. This library is used on non-ELF systems. > + /* Look for for the .gnu_debuglink section */ Period at end of sentence. >/* To translate PC to file/line when using DWARF, we need to find > - the .debug_info and .debug_line sections. */ > + the .debug_info and .debug_line sections. */ Why change the indentation like this? I think the original was correct. > - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, > - pd->data, &does_not_exist); > + descriptor > + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, > +&debugfile_does_not_exist, pd->state, 0); > + if (descriptor < 0) > + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, > + pd->data, &does_not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? > +/*Just a simple test copied from btest.c, but in this case we don't have > debug > + * info in the executable and test should verify that we can read debug info > + * from separate file. See Makefile check-TESTS target. */ No leading '*' on subsequent lines of multi-line comments, see existing code. I'm not sure I see the point of glinktest.c. Why don't we just use btest.c? > +#define MAX_PATH_LEN 4096 /* from linux/limits.h */ This library works on systems other than GNU/Linux. We should dynamically allocate the buffer. > + while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1 while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1])) or just call basename. I'm not sure why you bother with the count variable; doesn't full_filename_len - count exactly == len? > + sign_byte = 0x00; > + count = 0; > + while (*(buffer + count) != sign_byte && size > count) > +++count; > + return count; This looks like `return strnlen(buffer, size)`. Ian
Re: [PATCH][ARM] PR target/71436: Restrict *load_multiple pattern till after LRA
On 07/02/17 14:49, Kyrill Tkachov wrote:
On 18/01/17 09:49, Kyrill Tkachov wrote:
On 19/12/16 14:53, Jakub Jelinek wrote:
On Thu, Dec 15, 2016 at 10:00:14AM +, Richard Earnshaw (lists) wrote:
sorry, pasted the wrong bit of code.
That should read when we generate:
(insn 55 19 67 3 (parallel [
(set (reg:SI 0 r0)
(mem/u/c:SI (reg/f:SI 147) [2 c+0 S4 A32]))
(set (reg:SI 158)
(mem/u/c:SI (plus:SI (reg/f:SI 147)
(const_int 4 [0x4])) [2 c+4 S4 A32]))
]) "ldm.c":25 404 {*load_multiple}
(expr_list:REG_UNUSED (reg:SI 0 r0)
(nil)))
ie when we put a pseudo into the register load list.
We put a pseudo there because the predicate on the insn allows it:
(define_special_predicate "load_multiple_operation"
(match_code "parallel")
{
return ldm_stm_operation_p (op, /*load=*/true, SImode,
/*consecutive=*/false,
/*return_pc=*/false);
})
and the consecutive = false argument says that (almost) no verification
is performed on the SET_DEST, just that it is a REG and doesn't have
REGNO smaller than the first reg.
That said, RA is still not able to cope with such instructions, because
only the first set is represented with constraints, so if such an insn
needs any kind of reloading, it just will not happen.
So I think the posted patch makes lots of sense, otherwise if you use
such a pattern before reload, you just have to hope no reloading will be
needed on it.
In that case I believe restricting the pattern till after LRA is the way to go.
However, we should still add the check from [1] to ldm_stm_operation_p for when
'consecutive' is true as consecutive order has no useful meaning on pseudos
here.
In the interest of fixing this PR I think the patch at
https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03078.html
is the way to go, that is disable this pattern until after reload.
It was intended to handle epilogue pops that is generated after reload anyway
and all the general load/store multiple
patterns are handled by ldmstmd.md so not having this before reload is not
risky.
I'll also propose a variant of
https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01400.html to tighten up the
consecutivity
check when 'consecutive' is passed down as true, but that is indepdendent of
this PR.
Is the patch at https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03078.html ok for
trunk?
It's been in my tree for a couple of months now getting testing and there's
been no fallout.
Ping.
Thanks,
Kyrill
Thanks,
Kyrill
[1] https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01400.html
Jakub
Re: [doc] Omit up pointer (DIR) in gcc.texi and gccint.texi?
On Mon, 13 Mar 2017, Joseph Myers wrote: > I think that as far as possible we should just use "@node node-name" > without any of the subsequent arguments pointing to other nodes. Thanks, Joseph. I went ahead and applied the patch below. Gerald 2017-03-13 Gerald Pfeifer * doc/gcc.texi: Remove "up" link to (DIR). * doc/gccint.texi: Ditto. Index: doc/gcc.texi === --- doc/gcc.texi(revision 246109) +++ doc/gcc.texi(working copy) @@ -112,7 +112,7 @@ @contents @page -@node Top, G++ and GCC,, (DIR) +@node Top, G++ and GCC @top Introduction @cindex introduction Index: doc/gccint.texi === --- doc/gccint.texi (revision 246109) +++ doc/gccint.texi (working copy) @@ -70,7 +70,7 @@ @contents @page -@node Top, Contributing,, (DIR) +@node Top, Contributing @top Introduction @cindex introduction
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On 03/14/2017 11:48 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 11:30 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 11:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/14/2017 10:12 AM, Richard Biener wrote:
>>> On Tue, 14 Mar 2017, Martin Liška wrote:
>>>
On 03/14/2017 09:13 AM, Richard Biener wrote:
> On Tue, 14 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 04:16 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Martin Liška wrote:
>>>
On 03/13/2017 02:53 PM, Richard Biener wrote:
> On Mon, 13 Mar 2017, Martin Liška wrote:
>
>> On 03/13/2017 02:01 PM, Richard Biener wrote:
>>> On Mon, 13 Mar 2017, Richard Biener wrote:
>>>
On Fri, 10 Mar 2017, Martin Liška wrote:
> Hello.
>
> As briefly discussed in the PR, there are BB that do not
> correspond to a real
> line in source
> code. profile.c emits locations for all BBs that have a
> gimple statement
> belonging to a line.
> I hope these should be marked in gcov utility and not added
> in --all-block
> mode to counts of lines.
>
> Patch survives make check RUNTESTFLAGS="gcov.exp".
>
> Thanks for review and feedback.
Humm, the patch doesn't seem to change the BBs associated with
a line
but rather somehow changes how we compute counts (the patch
lacks a
description of how you arrived at it). I expected the
line-to-BB
assignment process to be changed (whereever that is...).
>>
>> Currently, each basic block must belong to a source line. It's
>> how gcov
>> iterates all blocks (via lines).
>>
>>>
>>> Ah, ok, looking at where output_location is called on I see we
>>> do not
>>> assign any line to that block. But still why does
>>> line->has_block (arc->src) return true?
>>
>> Good objection! Problematic that 4->5 edge really comes from
>> the same line.
>>
>>[0.00%]:
>> ret_7 = 111;
>> PROF_edge_counter_10 = __gcov0.UuT[0];
>> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
>> __gcov0.UuT[0] = PROF_edge_counter_11;
>>
>>[0.00%]:
>> # ret_1 = PHI
>> goto ; [0.00%]
>
> Yes, but that's basically meaningless, unless not all edges come
> from the
> same line? I see nowhere where we'd explicitely assign lines to
> edges so it must be gcov "reconstructing" this somewhere.
That's why I added the another flag. We stream locations for basic
blocks via
'output_location' function. And assignment blocks to lines happens
here:
static void
add_line_counts (coverage_t *coverage, function_t *fn)
{
...
if (!ix || ix + 1 == fn->num_blocks)
/* Entry or exit block */;
else if (flag_all_blocks)
{
line_t *block_line = line;
if (!block_line)
block_line = &sources[fn->src].lines[fn->line];
block->chain = block_line->u.blocks;
block_line->u.blocks = block;
}
where line is always changes when we reach a BB that has a source
line assigned. Thus it's changed
for BB 4 and that's why BB 5 is added to same line.
>>>
>>> Ah, so this means we should "clear" the current line for BB 5 in
>>> output_location? At least I don't see how your patch may not
>>> regress
>>> some cases where the line wasn't output as an optimization?
>>
>> The new flag on block is kind of clearing that the BB is artificial
>> and in fact does not
>> belong to the line. I didn't want to do a bigger refactoring how
>> blocks are iterated via lines.
>>
>> Can you be please more specific about such a case?
>
> in profile.c I see
>
> if (name_differs || line_differs)
> {
> if (!*offset)
> {
> *offset = gcov_write_tag (GCO
Re: [PATCH] Install gcov-dump.
On Tue, Mar 14, 2017 at 1:30 PM, Martin Liška wrote: > Tested on my local machine that's properly installed. > > Ready for trunk? Ok. Richard. > Thanks, > Martin
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
On Tue, 14 Mar 2017, Martin Liška wrote:
> On 03/14/2017 11:48 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 11:30 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/14/2017 11:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/14/2017 10:12 AM, Richard Biener wrote:
> >>> On Tue, 14 Mar 2017, Martin Liška wrote:
> >>>
> On 03/14/2017 09:13 AM, Richard Biener wrote:
> > On Tue, 14 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 04:16 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Martin Liška wrote:
> >>>
> On 03/13/2017 02:53 PM, Richard Biener wrote:
> > On Mon, 13 Mar 2017, Martin Liška wrote:
> >
> >> On 03/13/2017 02:01 PM, Richard Biener wrote:
> >>> On Mon, 13 Mar 2017, Richard Biener wrote:
> >>>
> On Fri, 10 Mar 2017, Martin Liška wrote:
>
> > Hello.
> >
> > As briefly discussed in the PR, there are BB that do not
> > correspond to a real
> > line in source
> > code. profile.c emits locations for all BBs that have a
> > gimple statement
> > belonging to a line.
> > I hope these should be marked in gcov utility and not added
> > in --all-block
> > mode to counts of lines.
> >
> > Patch survives make check RUNTESTFLAGS="gcov.exp".
> >
> > Thanks for review and feedback.
>
> Humm, the patch doesn't seem to change the BBs associated
> with a line
> but rather somehow changes how we compute counts (the patch
> lacks a
> description of how you arrived at it). I expected the
> line-to-BB
> assignment process to be changed (whereever that is...).
> >>
> >> Currently, each basic block must belong to a source line. It's
> >> how gcov
> >> iterates all blocks (via lines).
> >>
> >>>
> >>> Ah, ok, looking at where output_location is called on I see
> >>> we do not
> >>> assign any line to that block. But still why does
> >>> line->has_block (arc->src) return true?
> >>
> >> Good objection! Problematic that 4->5 edge really comes from
> >> the same line.
> >>
> >>[0.00%]:
> >> ret_7 = 111;
> >> PROF_edge_counter_10 = __gcov0.UuT[0];
> >> PROF_edge_counter_11 = PROF_edge_counter_10 + 1;
> >> __gcov0.UuT[0] = PROF_edge_counter_11;
> >>
> >>[0.00%]:
> >> # ret_1 = PHI
> >> goto ; [0.00%]
> >
> > Yes, but that's basically meaningless, unless not all edges
> > come from the
> > same line? I see nowhere where we'd explicitely assign lines to
> > edges so it must be gcov "reconstructing" this somewhere.
>
> That's why I added the another flag. We stream locations for
> basic blocks via
> 'output_location' function. And assignment blocks to lines
> happens here:
>
> static void
> add_line_counts (coverage_t *coverage, function_t *fn)
> {
> ...
> if (!ix || ix + 1 == fn->num_blocks)
> /* Entry or exit block */;
> else if (flag_all_blocks)
> {
> line_t *block_line = line;
>
> if (!block_line)
> block_line = &sources[fn->src].lines[fn->line];
>
> block->chain = block_line->u.blocks;
> block_line->u.blocks = block;
> }
>
> where line is always changes when we reach a BB that has a
> source line assigned. Thus it's changed
> for BB 4 and that's why BB 5 is added to same line.
> >>>
> >>> Ah, so this means we should "clear" the current line for BB 5 in
> >>> output_location? At least I don't see how your patch may not
> >>> regress
> >>> some cases where the line wasn't output as an optimization?
> >>
> >> The new flag on block is kind of clearing that the BB is
> >> artificial and in fact does not
> >> belong to the line. I didn't want to do a bigger refactoring how
> >> blocks are iterated via lines.
> >>
> >> Can y
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 7:50 AM, Bill Schmidt wrote: > > >> On Mar 14, 2017, at 3:57 AM, Richard Biener >> wrote: >> >> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt >> wrote: >>> >>> Index: gcc/tree-stdarg.c >>> === >>> --- gcc/tree-stdarg.c (revision 246109) >>> +++ gcc/tree-stdarg.c (working copy) >>> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun) >>> types. */ >>> gimplify_assign (lhs, expr, &pre); >>> } >>> - else >>> + else if (is_gimple_addressable (expr)) >>> gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >> >> This is wrong - we lose side-effects this way and the only reason we gimplify >> is to _not_ lose them. >> >> Better is sth like >> >> Index: gcc/tree-stdarg.c >> === >> --- gcc/tree-stdarg.c (revision 246082) >> +++ gcc/tree-stdarg.c (working copy) >> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) >> gimplify_assign (lhs, expr, &pre); >> } >> else >> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >> >> input_location = saved_location; >> pop_gimplify_context (NULL); > > OK, thanks for the explanation. Unfortunately this fails bootstrap with a > failed > assert a little later. I'll dig further. Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue passes. Trying this now: Index: gcc/tree-stdarg.c === --- gcc/tree-stdarg.c (revision 246109) +++ gcc/tree-stdarg.c (working copy) @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun) types. */ gimplify_assign (lhs, expr, &pre); } + else if (is_gimple_addressable (expr)) + gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); else - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); input_location = saved_location; pop_gimplify_context (NULL); > > Bill >
Re: [PATCH] Fix PR79908
On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt wrote: > >> On Mar 14, 2017, at 7:50 AM, Bill Schmidt >> wrote: >> >> >>> On Mar 14, 2017, at 3:57 AM, Richard Biener >>> wrote: >>> >>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt >>> wrote: Index: gcc/tree-stdarg.c === --- gcc/tree-stdarg.c (revision 246109) +++ gcc/tree-stdarg.c (working copy) @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun) types. */ gimplify_assign (lhs, expr, &pre); } - else + else if (is_gimple_addressable (expr)) gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>> >>> This is wrong - we lose side-effects this way and the only reason we >>> gimplify >>> is to _not_ lose them. >>> >>> Better is sth like >>> >>> Index: gcc/tree-stdarg.c >>> === >>> --- gcc/tree-stdarg.c (revision 246082) >>> +++ gcc/tree-stdarg.c (working copy) >>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) >>> gimplify_assign (lhs, expr, &pre); >>> } >>> else >>> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >>> >>> input_location = saved_location; >>> pop_gimplify_context (NULL); >> >> OK, thanks for the explanation. Unfortunately this fails bootstrap with a >> failed >> assert a little later. I'll dig further. > > Looks like is_gimple_val is too restrictive for MEM_REFs, for which > is_gimple_lvalue > passes. Trying this now: Hmm, it should simply gimplify to a load if it's not aggregate. Can you share a testcase where it doesn't work? > Index: gcc/tree-stdarg.c > === > --- gcc/tree-stdarg.c (revision 246109) > +++ gcc/tree-stdarg.c (working copy) > @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun) >types. */ > gimplify_assign (lhs, expr, &pre); > } > + else if (is_gimple_addressable (expr)) I believe any is_gimple_addressable check is flawed. > + gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > else > - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); > > input_location = saved_location; > pop_gimplify_context (NULL); > > >> >> Bill >> >
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for review, got all of my mistakes, except one. > - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, > - pd->data, &does_not_exist); > + descriptor > + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, > +&debugfile_does_not_exist, pd->state, 0); > + if (descriptor < 0) > + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, > + pd->data, &does_not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? Should i move code below /* check if debug section does exist */ debug_link_data = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe, &debug_link_data_len); from backtrace_open_debugfile . On 03/14/2017 04:49 PM, Ian Lance Taylor wrote: On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov wrote: Hello everyone, i have a patch for this issue. List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo +clean_separate:glinktest.debug + rm -f glinktest.debug + +separate: glinktest + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ + then \ + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ + $(STRIP) glinktest;\ + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ + fi; As far as I know "separate" is not a special thing in automake. We should always run (and clean) this test if the necessary objcopy option is available. +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h Missing '$' in "$(INCDIR)". +/* Return 1 if header is valid and -1 on fail */ This comment does not explain what the function actually does. +/* Return the pointer to char array with data from .gnudebuglink section inside. */ Line too long, we use 80 column lines. +unsigned char * +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor, + backtrace_error_callback error_callback, void *data, + int exe, int *gnulink_data_len_out) This should be static. I see that you are calling it elsewhere, but it doesn't make sense to call an "elf" function outside of elf.c. This library is used on non-ELF systems. + /* Look for for the .gnu_debuglink section */ Period at end of sentence. /* To translate PC to file/line when using DWARF, we need to find - the .debug_info and .debug_line sections. */ + the .debug_info and .debug_line sections. */ Why change the indentation like this? I think the original was correct. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, &does_not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +&debugfile_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd->data, &does_not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? +/*Just a simple test copied from btest.c, but in this case we don't have debug + * info in the executable and test should verify that we can read debug info + * from separate file. See Makefile check-TESTS target. */ No leading '*' on subsequent lines of multi-line comments, see existing code. I'm not sure I see the point of glinktest.c. Why don't we just use btest.c? +#define MAX_PATH_LEN 4096 /* from linux/limits.h */ This library works on systems other than GNU/Linux. We should dynamically allocate the buffer. + while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1 while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1])) or just call basename. I'm not sure why you bother with the count variable; doesn't full_filename_len - count exactly == len? + sign_byte = 0x00; + count = 0; + while (*(buffer + count) != sign_byte && size > count) +++count; + return count; This looks like `return strnlen(buffer, size)`. Ian
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 9:25 AM, Richard Biener > wrote: > > On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt > wrote: >> >>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt >>> wrote: >>> >>> On Mar 14, 2017, at 3:57 AM, Richard Biener wrote: On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt wrote: > > Index: gcc/tree-stdarg.c > === > --- gcc/tree-stdarg.c (revision 246109) > +++ gcc/tree-stdarg.c (working copy) > @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun) > types. */ > gimplify_assign (lhs, expr, &pre); >} > - else > + else if (is_gimple_addressable (expr)) >gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); This is wrong - we lose side-effects this way and the only reason we gimplify is to _not_ lose them. Better is sth like Index: gcc/tree-stdarg.c === --- gcc/tree-stdarg.c (revision 246082) +++ gcc/tree-stdarg.c (working copy) @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) gimplify_assign (lhs, expr, &pre); } else - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); input_location = saved_location; pop_gimplify_context (NULL); >>> >>> OK, thanks for the explanation. Unfortunately this fails bootstrap with a >>> failed >>> assert a little later. I'll dig further. >> >> Looks like is_gimple_val is too restrictive for MEM_REFs, for which >> is_gimple_lvalue >> passes. Trying this now: > > Hmm, it should simply gimplify to a load if it's not aggregate. Can > you share a testcase > where it doesn't work? Your suggestion failed bootstrap in libiberty on vprintf-support.c. Compilation failed with: /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/ -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include -c -DHAVE_CONFIG_H -g -O2 -gtoggle -I. -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic -D_GNU_SOURCE -fPIC /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o pic/vprintf-support.o The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)). Gimplification turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test fails on that. Bill > >> Index: gcc/tree-stdarg.c >> === >> --- gcc/tree-stdarg.c (revision 246109) >> +++ gcc/tree-stdarg.c (working copy) >> @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun) >> types. */ >>gimplify_assign (lhs, expr, &pre); >> } >> + else if (is_gimple_addressable (expr)) > > I believe any is_gimple_addressable check is flawed. OK, just wanted to try something quick and dirty before your end of day. Not sure how else to deal with this. Bill > >> + gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>else >> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); >> >>input_location = saved_location; >>pop_gimplify_context (NULL); >> >> >>> >>> Bill
Re: [C++ PATCH] 79393 fix
On 03/13/2017 06:20 PM, Nathan Sidwell wrote:
On 03/13/2017 05:05 PM, Jason Merrill wrote:
It looks like you're ignoring the access for all base destructors;
handling this in synthesized_method_base_walk would let you limit the
change to vbases with virtual destructors. That function also already
handles ignoring access control for an inherited constructor.
Committed this. It looked to me that the inheriting ctor case should be
using dk_no_check rather than dk_deferring, given the comment. But I
didn't want to change that in this patch.
nathan
--
Nathan Sidwell
2017-03-14 Nathan Sidwell
PR c++/79393 DR 1658 workaround
* method.c (synthesized_method_base_walk): Inihibit abstract class
virtual base access check here.
(synthesized_method_walk): Not here.
Index: cp/method.c
===
--- cp/method.c (revision 246120)
+++ cp/method.c (working copy)
@@ -1420,10 +1420,10 @@ walk_field_subobs (tree fields, tree fnn
}
}
-// Base walker helper for synthesized_method_walk. Inspect a direct
-// or virtual base. BINFO is the parent type's binfo. BASE_BINFO is
-// the base binfo of interests. All other parms are as for
-// synthesized_method_walk, or its local vars.
+/* Base walker helper for synthesized_method_walk. Inspect a direct
+ or virtual base. BINFO is the parent type's binfo. BASE_BINFO is
+ the base binfo of interests. All other parms are as for
+ synthesized_method_walk, or its local vars. */
static tree
synthesized_method_base_walk (tree binfo, tree base_binfo,
@@ -1436,7 +1436,8 @@ synthesized_method_base_walk (tree binfo
{
bool inherited_binfo = false;
tree argtype = NULL_TREE;
-
+ deferring_kind defer = dk_no_deferred;
+
if (copy_arg_p)
argtype = build_stub_type (BINFO_TYPE (base_binfo), quals, move_p);
else if ((inherited_binfo
@@ -1445,11 +1446,21 @@ synthesized_method_base_walk (tree binfo
argtype = inherited_parms;
/* Don't check access on the inherited constructor. */
if (flag_new_inheriting_ctors)
- push_deferring_access_checks (dk_deferred);
+ defer = dk_deferred;
}
+ /* To be conservative, ignore access to the base dtor that
+ DR1658 instructs us to ignore. See the comment in
+ synthesized_method_walk. */
+ else if (cxx_dialect >= cxx14 && fnname == complete_dtor_identifier
+ && BINFO_VIRTUAL_P (base_binfo)
+ && ABSTRACT_CLASS_TYPE_P (BINFO_TYPE (binfo)))
+defer = dk_no_check;
+
+ if (defer != dk_no_deferred)
+push_deferring_access_checks (defer);
tree rval = locate_fn_flags (base_binfo, fnname, argtype, flags,
diag ? tf_warning_or_error : tf_none);
- if (inherited_binfo && flag_new_inheriting_ctors)
+ if (defer != dk_no_deferred)
pop_deferring_access_checks ();
process_subob_fn (rval, spec_p, trivial_p, deleted_p,
@@ -1677,13 +1688,6 @@ synthesized_method_walk (tree ctype, spe
if (constexpr_p)
*constexpr_p = false;
- /* To be conservative, ignore access to the base dtor that
- DR1658 instructs us to ignore. */
- bool no_access_check = (cxx_dialect >= cxx14
- && ABSTRACT_CLASS_TYPE_P (ctype));
-
- if (no_access_check)
- push_deferring_access_checks (dk_no_check);
FOR_EACH_VEC_ELT (*vbases, i, base_binfo)
synthesized_method_base_walk (binfo, base_binfo, quals,
copy_arg_p, move_p, ctor_p,
@@ -1691,8 +1695,6 @@ synthesized_method_walk (tree ctype, spe
fnname, flags, diag,
spec_p, trivial_p,
deleted_p, constexpr_p);
- if (no_access_check)
- pop_deferring_access_checks ();
}
/* Now handle the non-static data members. */
Index: testsuite/g++.dg/cpp1y/pr79393-2.C
===
--- testsuite/g++.dg/cpp1y/pr79393-2.C (revision 0)
+++ testsuite/g++.dg/cpp1y/pr79393-2.C (working copy)
@@ -0,0 +1,22 @@
+// { dg-do compile { target c++14 } }
+
+// DR 1658, inaccessible dtor of virtual base doesn't affect an
+// abstract class. But we should stil check access to non-virtual bases.
+
+class C;
+
+struct A {
+private:
+ ~A (){ }
+ friend class C;
+};
+
+struct B : A { // { dg-error "is private" }
+ virtual bool Ok () = 0; // abstract
+};
+
+struct C : B {
+ ~C ()
+ { } // { dg-error "use of deleted" }
+ virtual bool Ok ();
+};
[PATCH][PR target/79752] fix rs6000 power9 peephole2 for udiv/umod -- backported to gcc-6-branch
This showed up in power9 code for __divkf3 software float support and caused a divd to be emitted where we needed a divdu. backported/bootstrapped/regtested to gcc-6-branch Index: gcc/config/rs6000/rs6000.md === --- gcc/config/rs6000/rs6000.md (revision 246123) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -3063,8 +3063,8 @@ && ! reg_mentioned_p (operands[3], operands[1]) && ! reg_mentioned_p (operands[3], operands[2])" [(set (match_dup 0) - (div:GPR (match_dup 1) -(match_dup 2))) + (udiv:GPR (match_dup 1) + (match_dup 2))) (set (match_dup 3) (mult:GPR (match_dup 0) (match_dup 2))) 2017-03-14 Aaron Sawdey Backport from mainline 2017-02-28 Aaron Sawdey PR target/79752 * config/rs6000/rs6000.md (peephole2 for udiv/umod): Should emit udiv rather than div since input pattern is unsigned. -- Aaron Sawdey, Ph.D. acsaw...@linux.vnet.ibm.com 050-2/C113 (507) 253-7520 home: 507/263-0782 IBM Linux Technology Center - PPC Toolchain
[PATCH] PR libstdc++/79162 disambiguate assignment from string_view
This fixes a new ambiguity in C++17 mode introduced by string_view
support in basic_string.
PR libstdc++/79162
* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(basic_string_view)): Replace
with a constrained template.
[!_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(basic_string_view)): Likewise.
* testsuite/21_strings/basic_string/cons/char/79162.cc: New test.
* testsuite/21_strings/basic_string/cons/wchar_t/79162.cc: New test.
Tested ppc64le-linux, committed to trunk.
commit 14b75fa34a68bfbc60fc20685bcb19e152b4cc56
Author: Jonathan Wakely
Date: Tue Mar 14 13:47:12 2017 +
PR libstdc++/79162 disambiguate assignment from string_view
PR libstdc++/79162
* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(basic_string_view)): Replace
with a constrained template.
[!_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(basic_string_view)): Likewise.
* testsuite/21_strings/basic_string/cons/char/79162.cc: New test.
* testsuite/21_strings/basic_string/cons/wchar_t/79162.cc: New test.
diff --git a/libstdc++-v3/include/bits/basic_string.h
b/libstdc++-v3/include/bits/basic_string.h
index 981ffc5..b6693c4 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -758,9 +758,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
* @brief Set value to string constructed from a string_view.
* @param __sv A string_view.
*/
- basic_string&
- operator=(__sv_type __sv)
- {return this->assign(__sv); }
+ template
+ _If_sv<_Tp, basic_string&>
+ operator=(_Tp __sv)
+ { return this->assign(__sv); }
/**
* @brief Convert to a string_view.
@@ -3560,9 +3561,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
* @brief Set value to string constructed from a string_view.
* @param __sv A string_view.
*/
- basic_string&
- operator=(__sv_type __sv)
- { return this->assign(__sv); }
+ template
+ _If_sv<_Tp, basic_string&>
+ operator=(_Tp __sv)
+ { return this->assign(__sv); }
/**
* @brief Convert to a string_view.
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/79162.cc
b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/79162.cc
new file mode 100644
index 000..0afbe11
--- /dev/null
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/79162.cc
@@ -0,0 +1,28 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+//
[PATCH][DOC] Document -Wchkp (PR middle-end/79831).
Hello.
This is small documentation patch that lists and briefly describes -Wchkp.
Martin
>From 553e0c83a1b1efbaff7666f5ab7303e816b958d4 Mon Sep 17 00:00:00 2001
From: marxin
Date: Tue, 14 Mar 2017 15:41:47 +0100
Subject: [PATCH] Document -Wchkp (PR middle-end/79831).
gcc/ChangeLog:
2017-03-14 Martin Liska
PR middle-end/79831
* doc/invoke.texi (-Wchkp): Document the option.
---
gcc/doc/invoke.texi | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 2852642516b..04ce03de28c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -265,7 +265,8 @@ Objective-C and Objective-C++ Dialects}.
-Wno-builtin-declaration-mismatch @gol
-Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol
-Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol
--Wchar-subscripts -Wclobbered -Wcomment -Wconditionally-supported @gol
+-Wchar-subscripts -Wchkp -Wclobbered -Wcomment @gol
+-Wconditionally-supported @gol
-Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol
-Wdelete-incomplete @gol
-Wno-deprecated -Wno-deprecated-declarations -Wno-designated-init @gol
@@ -3860,6 +3861,11 @@ of error, as programmers often forget that this type is signed on some
machines.
This warning is enabled by @option{-Wall}.
+@item -Wchkp
+@opindex Wchkp
+Warn about an invalid memory access that is found by Pointer Bounds Checker
+(@option{-fcheck-pointer-bounds}).
+
@item -Wno-coverage-mismatch
@opindex Wno-coverage-mismatch
Warn if feedback profiles do not match when using the
--
2.11.1
[PATCH][DOC] Document options that can't be combined with -fcheck-pointer-bounds.
Hello.
This mentions all options that can't be combined with -fcheck-pointer-bounds.
Thanks,
Martin
>From 78e199c5f0359de1c74cf44aaa8947a0ae950dd3 Mon Sep 17 00:00:00 2001
From: marxin
Date: Tue, 14 Mar 2017 16:00:03 +0100
Subject: [PATCH] Document options that can't be combined with
-fcheck-pointer-bounds.
gcc/ChangeLog:
2017-03-14 Martin Liska
* doc/invoke.texi: Document options that can't be combined with
-fcheck-pointer-bounds.
---
gcc/doc/invoke.texi | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 04ce03de28c..cda570d73ff 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10733,12 +10733,14 @@ more details. The run-time behavior can be influenced using the
the available options are shown at startup of the instrumented program. See
@url{https://github.com/google/sanitizers/wiki/AddressSanitizerFlags#run-time-flags}
for a list of supported options.
-The option can't be combined with @option{-fsanitize=thread}.
+The option can't be combined with @option{-fsanitize=thread}
+and/or @option{-fcheck-pointer-bounds}.
@item -fsanitize=kernel-address
@opindex fsanitize=kernel-address
Enable AddressSanitizer for Linux kernel.
See @uref{https://github.com/google/kasan/wiki} for more details.
+The option can't be combined with @option{-fcheck-pointer-bounds}.
@item -fsanitize=thread
@opindex fsanitize=thread
@@ -10749,8 +10751,8 @@ details. The run-time behavior can be influenced using the @env{TSAN_OPTIONS}
environment variable; see
@url{https://github.com/google/sanitizers/wiki/ThreadSanitizerFlags} for a list of
supported options.
-The option can't be combined with @option{-fsanitize=address}
-and/or @option{-fsanitize=leak}.
+The option can't be combined with @option{-fsanitize=address},
+@option{-fsanitize=leak} and/or @option{-fcheck-pointer-bounds}.
@item -fsanitize=leak
@opindex fsanitize=leak
@@ -10839,13 +10841,15 @@ a++;
This option enables instrumentation of array bounds. Various out of bounds
accesses are detected. Flexible array members, flexible array member-like
arrays, and initializers of variables with static storage are not instrumented.
+The option can't be combined with @option{-fcheck-pointer-bounds}.
@item -fsanitize=bounds-strict
@opindex fsanitize=bounds-strict
This option enables strict instrumentation of array bounds. Most out of bounds
accesses are detected, including flexible array members and flexible array
member-like arrays. Initializers of variables with static storage are not
-instrumented.
+instrumented. The option can't be combined
+with @option{-fcheck-pointer-bounds}.
@item -fsanitize=alignment
@opindex fsanitize=alignment
--
2.11.1
Re: [PATCH][DOC] Document options that can't be combined with -fcheck-pointer-bounds.
On 03/14/2017 09:02 AM, Martin Liška wrote: Hello. This mentions all options that can't be combined with -fcheck-pointer-bounds. OK. jeff
Re: [PATCH][DOC] Document -Wchkp (PR middle-end/79831).
On 03/14/2017 09:01 AM, Martin Liška wrote: Hello. This is small documentation patch that lists and briefly describes -Wchkp. OK. jeff
Re: [PATCH] add calls.c to GTFILES in Makefile.in (PR 79936)
On 03/14/2017 03:01 AM, Richard Biener wrote: On Tue, Mar 14, 2017 at 1:37 AM, Martin Sebor wrote: Ping: this a P3 regression targeted at 7.0.1. It was found on x86_64-apple-darwin10.8.0 but affects all targets (even if it doesn't happen to manifest on them): https://gcc.gnu.org/ml/gcc-patches/2017-03/msg00342.html I think you need to include gt-calls.h at the bottom of the file. See other GTFILES But the patch does include gt-calls.h at the end of calls.c, which is consistent with other uses. Or did you have something else in mind? jeff
Re: [C++ PATCH] 79393 fix
On Tue, Mar 14, 2017 at 10:43 AM, Nathan Sidwell wrote: > On 03/13/2017 06:20 PM, Nathan Sidwell wrote: >> >> On 03/13/2017 05:05 PM, Jason Merrill wrote: >> >>> It looks like you're ignoring the access for all base destructors; >>> handling this in synthesized_method_base_walk would let you limit the >>> change to vbases with virtual destructors. That function also already >>> handles ignoring access control for an inherited constructor. > > > Committed this. It looked to me that the inheriting ctor case should be > using dk_no_check rather than dk_deferring, given the comment. But I didn't > want to change that in this patch. The difference is that dk_no_check overrides any further pushes; see push_deferring_access_checks. I don't remember why that is, but that's why we instead use dk_deferring and then throw away the deferred checks. Jason
Re: [PATCH] Fix PR79908
On Mar 14, 2017, at 9:32 AM, Bill Schmidt wrote:
>
>> On Mar 14, 2017, at 9:25 AM, Richard Biener
>> wrote:
>>
> Better is sth like
>
> Index: gcc/tree-stdarg.c
> ===
> --- gcc/tree-stdarg.c (revision 246082)
> +++ gcc/tree-stdarg.c (working copy)
> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun)
> gimplify_assign (lhs, expr, &pre);
> }
> else
> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>
> input_location = saved_location;
> pop_gimplify_context (NULL);
OK, thanks for the explanation. Unfortunately this fails bootstrap with a
failed
assert a little later. I'll dig further.
>>>
>>> Looks like is_gimple_val is too restrictive for MEM_REFs, for which
>>> is_gimple_lvalue
>>> passes. Trying this now:
>>
>> Hmm, it should simply gimplify to a load if it's not aggregate. Can
>> you share a testcase
>> where it doesn't work?
>
> Your suggestion failed bootstrap in libiberty on vprintf-support.c.
> Compilation failed with:
>
> /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc
> -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/
> -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/
>
> -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/
>
> -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/
> -isystem
> /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include
> -isystem
> /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include
> -c -DHAVE_CONFIG_H -g -O2 -gtoggle -I.
> -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include -W -Wall
> -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic
> -D_GNU_SOURCE -fPIC
> /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o
> pic/vprintf-support.o
>
> The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)).
> Gimplification
> turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test
> fails on that.
Reduced test case:
typedef __builtin_va_list __gnuc_va_list;
typedef __gnuc_va_list va_list;
void
foo (va_list args)
{
va_list ap;
__builtin_va_copy (ap, args);
(void)__builtin_va_arg (ap, int);
__builtin_va_end(ap);
}
C++ backports to 6
I've backported the attached patches gcc-6.
Marek
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2017-03-14 Marek Polacek
PR c++/79962
PR c++/79984
* c-common.c (handle_nonnull_attribute): Save the result of default
conversion to the attribute list.
* c-c++-common/nonnull-3.c: New test.
* g++.dg/warn/Wnonnull3.C: New test.
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index d2e3ad4..5f17984 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -9061,7 +9061,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED
(name),
tree arg = TREE_VALUE (args);
if (arg && TREE_CODE (arg) != IDENTIFIER_NODE
&& TREE_CODE (arg) != FUNCTION_DECL)
- arg = default_conversion (arg);
+ TREE_VALUE (args) = arg = default_conversion (arg);
if (!get_nonnull_operand (arg, &arg_num))
{
diff --git gcc/testsuite/c-c++-common/nonnull-3.c
gcc/testsuite/c-c++-common/nonnull-3.c
index e69de29..d2ccb24 100644
--- gcc/testsuite/c-c++-common/nonnull-3.c
+++ gcc/testsuite/c-c++-common/nonnull-3.c
@@ -0,0 +1,11 @@
+/* PR c++/79984 */
+/* { dg-do compile } */
+/* { dg-options "-Wnonnull-compare" } */
+
+enum { r = 1 };
+
+__attribute__ ((nonnull (r))) int
+f (int *p)
+{
+ return p == 0; /* { dg-warning "nonnull argument 'p' compared to NULL" } */
+}
diff --git gcc/testsuite/g++.dg/warn/Wnonnull3.C
gcc/testsuite/g++.dg/warn/Wnonnull3.C
index e69de29..d1918ef 100644
--- gcc/testsuite/g++.dg/warn/Wnonnull3.C
+++ gcc/testsuite/g++.dg/warn/Wnonnull3.C
@@ -0,0 +1,15 @@
+// PR c++/79962
+// { dg-options "-Wnonnull" }
+
+template
+__attribute__ ((__nonnull__ (T::i))) void f (typename T::U) { }
+
+struct S1 { enum { i = 1 }; typedef void* U; };
+struct S2 { static const int i = 1; typedef void* U; };
+
+void
+g ()
+{
+ f(0); // { dg-warning "null argument where non-null required" }
+ f(0); // { dg-warning "null argument where non-null required" }
+}
We crash on an assert in strip_typedefs, because we find ourselves in a
scenario where RESULT, the main variant of a struct, was modified in
finalize_record_size (its TYPE_ALIGN changed), but its variants (T in
strip_typedefs) weren't fixed-up yet; that will happen slightly later in
finalize_type_size. So there's a discrepancy that confuses the code.
This patch implements what Jason suggested to me on IRC, i.e., skip the
attribute handling if RESULT is complete and T isn't.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2017-03-09 Marek Polacek
PR c++/79900 - ICE in strip_typedefs
* tree.c (strip_typedefs): Skip the attribute handling if T is
a variant type which hasn't been updated yet.
* g++.dg/warn/Wpadded-1.C: New test.
diff --git gcc/cp/tree.c gcc/cp/tree.c
index d3c63b8..b3a4a10 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -1548,29 +1548,40 @@ strip_typedefs (tree t, bool *remove_attributes)
result = TYPE_MAIN_VARIANT (t);
}
gcc_assert (!typedef_variant_p (result));
- if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
- || TYPE_ALIGN (t) != TYPE_ALIGN (result))
+
+ if (COMPLETE_TYPE_P (result) && !COMPLETE_TYPE_P (t))
+ /* If RESULT is complete and T isn't, it's likely the case that T
+ is a variant of RESULT which hasn't been updated yet. Skip the
+ attribute handling. */;
+ else
{
- gcc_assert (TYPE_USER_ALIGN (t));
- if (remove_attributes)
- *remove_attributes = true;
- else
+ if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
+ || TYPE_ALIGN (t) != TYPE_ALIGN (result))
{
- if (TYPE_ALIGN (t) == TYPE_ALIGN (result))
- result = build_variant_type_copy (result);
+ gcc_assert (TYPE_USER_ALIGN (t));
+ if (remove_attributes)
+ *remove_attributes = true;
else
- result = build_aligned_type (result, TYPE_ALIGN (t));
- TYPE_USER_ALIGN (result) = true;
+ {
+ if (TYPE_ALIGN (t) == TYPE_ALIGN (result))
+ result = build_variant_type_copy (result);
+ else
+ result = build_aligned_type (result, TYPE_ALIGN (t));
+ TYPE_USER_ALIGN (result) = true;
+ }
+ }
+
+ if (TYPE_ATTRIBUTES (t))
+ {
+ if (remove_attributes)
+ result = apply_identity_attributes (result, TYPE_ATTRIBUTES (t),
+ remove_attributes);
+ else
+ result = cp_build_type_attribute_variant (result,
+ TYPE_ATTRIBUTES (t));
}
}
- if (TYPE_ATTRIBUTES (t))
-{
- if (remove_attributes)
- result = apply_identity_attributes (result, TYPE_ATTRIBUTES (t),
- remove_attributes);
- else
- result = cp_build_type_attribute_variant (result, TYPE_ATTRIBUTES (t));
-}
+
return cp_build_qualified_typ
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
On Tue, Mar 14, 2017 at 7:30 AM, Denis Khalikov wrote: > Thanks for review, got all of my mistakes, except one. > > >> - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, >> - pd->data, &does_not_exist); >> + descriptor >> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, >> pd->data, >> +&debugfile_does_not_exist, pd->state, 0); >> + if (descriptor < 0) >> + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, >> + pd->data, &does_not_exist); > > This seems like unnecessary work. Shouldn't we only try to open the > debug file if we find a .gnu_debuglink section? > > Should i move code below > > /* check if debug section does exist */ > debug_link_data > = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe, > &debug_link_data_len); > > from backtrace_open_debugfile . As far as I know all the debuglink code is ELF-specific. I would do it all in elf.c. While reading the sections of the executable, look for a debuglink section, and use it if present. Keep the readlink code in posix.c, I suppose. Apologies if this doesn't make sense. Ian > On 03/14/2017 04:49 PM, Ian Lance Taylor wrote: >> >> On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov >> wrote: >>> >>> Hello everyone, i have a patch for this issue. >>> >>> List of implemented functionality: >>> >>> 1.Reading .gnu_debuglink section from ELF file: >>> a. Reading name of debug info file. >>> b. Verifying crc32 sum. >>> >>> 2. Searching for separate debug info file from paths: >>> a. /usr/lib/debug/path/to/executable >>> b. /path/to/executable >>> c. /path/to/executable/.debug >>> >>> Assumed that debug info file generated by objcopy from binutils. >>> >>> objcopy --only-keep-debug foo foo.debug >>> strip -g foo >>> objcopy --add-gnu-debuglink=foo.debug foo >> >> >>> +clean_separate:glinktest.debug >>> + rm -f glinktest.debug >>> + >>> +separate: glinktest >>> + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ >>> + then \ >>> + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ >>> + $(STRIP) glinktest;\ >>> + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ >>> + fi; >> >> >> As far as I know "separate" is not a special thing in automake. We >> should always run (and clean) this test if the necessary objcopy >> option is available. >> >>> +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h >> >> >> Missing '$' in "$(INCDIR)". >> >>> +/* Return 1 if header is valid and -1 on fail */ >> >> >> This comment does not explain what the function actually does. >> >>> +/* Return the pointer to char array with data from .gnudebuglink section >>> inside. */ >> >> >> Line too long, we use 80 column lines. >> >>> +unsigned char * >>> +elf_gnu_debuglink_section (struct backtrace_state *state, int >>> descriptor, >>> + backtrace_error_callback error_callback, void *data, >>> + int exe, int *gnulink_data_len_out) >> >> >> This should be static. I see that you are calling it elsewhere, but >> it doesn't make sense to call an "elf" function outside of elf.c. >> This library is used on non-ELF systems. >> >>> + /* Look for for the .gnu_debuglink section */ >> >> >> Period at end of sentence. >> >>>/* To translate PC to file/line when using DWARF, we need to find >>> - the .debug_info and .debug_line sections. */ >>> + the .debug_info and .debug_line sections. */ >> >> >> Why change the indentation like this? I think the original was correct. >> >>> - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, >>> - pd->data, &does_not_exist); >>> + descriptor >>> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, >>> pd->data, >>> +&debugfile_does_not_exist, pd->state, 0); >>> + if (descriptor < 0) >>> + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, >>> + pd->data, &does_not_exist); >> >> >> This seems like unnecessary work. Shouldn't we only try to open the >> debug file if we find a .gnu_debuglink section? >> >>> +/*Just a simple test copied from btest.c, but in this case we don't have >>> debug >>> + * info in the executable and test should verify that we can read debug >>> info >>> + * from separate file. See Makefile check-TESTS target. */ >> >> >> No leading '*' on subsequent lines of multi-line comments, see existing >> code. >> >> I'm not sure I see the point of glinktest.c. Why don't we just use >> btest.c? >> >>> +#define MAX_PATH_LEN 4096 /* from linux/limits.h */ >> >> >> This library works on systems other than GNU/Linux. We should >> dynamically allocate the buffer. >> >>> + while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1 >> >> >> while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1])) >> >> or just call basename. I'm not sure why you bother with the count >> variable; doesn't full_filename_len - count exactly == len? >> >>> + sign_byte = 0x00; >>> + count = 0; >>> + while (*(buffer + count) != sign_
Re: terminology: zero character vs. null character
On 03/13/2017 04:34 PM, Bruce Korb wrote: On 03/13/17 15:02, Gerald Pfeifer wrote: On Mon, 13 Mar 2017, Joseph Myers wrote: I am currently translating GCC into German. During that, I noticed that in some places the term "zero character" means '\0'. The official term though is "null character", as per the C standard. Joseph, do you also agree (and with the patch below to document this)? Yes. Cool; I committed the change to codingconventions.html . I'm likely late to the party, but what's wrong with the traditional "NUL"? Googling "NUL vs. NULL" yields up: NULL is a macro defined in for the null pointer. NUL is the name of the first character in the ASCII character set. It corresponds to a zero value. There s no standard macro NUL in C, but some people like to define it. Personally, I think the term "NUL character" is fine. It's the abbreviation for the null character in both ASCII and EBCDIC, and it's used widely by POSIX, and interchangeably with the "null character." The C and C++ standards use the term "null character" exclusively, so if the GCC documentation should adopt one of them for consistency, it makes sense to go with "null." Martin
Re: [PATCH] Install gcov-dump.
On 14.03.2017 15:15, Richard Biener wrote: > On Tue, Mar 14, 2017 at 1:30 PM, Martin Liška wrote: >> Tested on my local machine that's properly installed. >> >> Ready for trunk? > > Ok. > > Richard. shouldn't that go to the active branches as well? Matthias
Re: [PATCH] Use fixed_nonglobal_reg_set to deduce ok_regs (PR rtl-optimization/79728)
On 03/06/2017 01:27 AM, Xi Ruoyao wrote: Hi, After Bernd fixed PR44281 (r235809), the registers fixed only because they are global can be selected by IRA. However, since they are not in ok_regs, the move cost estimation about them are wrong. Then an assertion in ira.c failed and then ICE. To fix this, add these registers into ok_regs. Bootstrapped/regtested on x86_64-linux-gnu. Is this ok for trunk? 2017-03-06 Xi Ruoyao PR rtl-optimization/79728 * reginfo.c (init_reg_sets_1): Use fixed_nonglobal_reg_set (instead of fixed_regs) to deduce ok_regs. * gcc.target/i386/pr79728.c: New test. I think I prefer Bernd's patch, mostly because it preserves the old meaning of the code for reload. jeff
Re: Fix IRA issue, PR79728
On 03/03/2017 06:51 AM, Bernd Schmidt wrote: This is an ICE where setup_pressure_classes fails if xmm0 is a global reg. Instead of GENERAL/FLOAT/SSE/MMX_REGS, it computes only SSE_FIRST_REG as the third register class. The problem is that the costs for moving between SSE_FIRST_REG and SSE_REGS are inflated because we think we have no available registers in SSE_FIRST_REG (since the only register in that class is global), and that somewhat confuses the algorithm. The following fixes it by tweaking contains_regs_of_mode. Out of caution, I've retained the old meaning for code in reload which uses this. Bootstrapped and tested on x86_64-linux. Ok? Bernd global-reg-cost.diff PR rtl-optimization/79728 * regs.h (struct target_regs): New field x_contains_allocatable_regs_of_mode. (contains_allocatable_regs_of_mode): New macro. * reginfo.c (init_reg_sets_1): Initialize it, and change contains_reg_of_mode so it includes global regs as well. * reload.c (push_reload): Use contains_allocatable_regs_of_mode rather thanc ontains_regs_of_mode. PR rtl-optimization/79728 * gcc.target/i386/sse-globalreg.c: New test. OK. jeff
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for answer, i got it now. I will also delete all readlink code. Looks like is no reason to use it instead just one call of realpath(char*,char*). Binutils using realpath in the same cases. On 03/14/2017 07:26 PM, Ian Lance Taylor wrote: On Tue, Mar 14, 2017 at 7:30 AM, Denis Khalikov wrote: Thanks for review, got all of my mistakes, except one. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, &does_not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +&debugfile_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd->data, &does_not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? Should i move code below /* check if debug section does exist */ debug_link_data = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe, &debug_link_data_len); from backtrace_open_debugfile . As far as I know all the debuglink code is ELF-specific. I would do it all in elf.c. While reading the sections of the executable, look for a debuglink section, and use it if present. Keep the readlink code in posix.c, I suppose. Apologies if this doesn't make sense. Ian On 03/14/2017 04:49 PM, Ian Lance Taylor wrote: On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov wrote: Hello everyone, i have a patch for this issue. List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo +clean_separate:glinktest.debug + rm -f glinktest.debug + +separate: glinktest + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ + then \ + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ + $(STRIP) glinktest;\ + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ + fi; As far as I know "separate" is not a special thing in automake. We should always run (and clean) this test if the necessary objcopy option is available. +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h Missing '$' in "$(INCDIR)". +/* Return 1 if header is valid and -1 on fail */ This comment does not explain what the function actually does. +/* Return the pointer to char array with data from .gnudebuglink section inside. */ Line too long, we use 80 column lines. +unsigned char * +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor, + backtrace_error_callback error_callback, void *data, + int exe, int *gnulink_data_len_out) This should be static. I see that you are calling it elsewhere, but it doesn't make sense to call an "elf" function outside of elf.c. This library is used on non-ELF systems. + /* Look for for the .gnu_debuglink section */ Period at end of sentence. /* To translate PC to file/line when using DWARF, we need to find - the .debug_info and .debug_line sections. */ + the .debug_info and .debug_line sections. */ Why change the indentation like this? I think the original was correct. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, &does_not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +&debugfile_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd->data, &does_not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? +/*Just a simple test copied from btest.c, but in this case we don't have debug + * info in the executable and test should verify that we can read debug info + * from separate file. See Makefile check-TESTS target. */ No leading '*' on subsequent lines of multi-line comments, see existing code. I'm not sure I see the point of glinktest.c. Why don't we just use btest.c? +#define MAX_PATH_LEN 4096 /* from linux/limits.h */ This library works on systems other than GNU/Linux. We should dynamically allocate the buffer. + while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1 while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1])) or just call basename. I'm not sure why you bother with the count variable; doesn't full_filename_len - count exactly == len? + sign_byte = 0x00; + count = 0; + while (*(buffer + count) != sign_byte && size > count) +++count; + return count; This looks like `return strnlen(buffer, size)`. Ian
Re: Fix IRA issue, PR79728
On 03/03/2017 06:51 AM, Bernd Schmidt wrote: This is an ICE where setup_pressure_classes fails if xmm0 is a global reg. Instead of GENERAL/FLOAT/SSE/MMX_REGS, it computes only SSE_FIRST_REG as the third register class. The problem is that the costs for moving between SSE_FIRST_REG and SSE_REGS are inflated because we think we have no available registers in SSE_FIRST_REG (since the only register in that class is global), and that somewhat confuses the algorithm. The following fixes it by tweaking contains_regs_of_mode. Out of caution, I've retained the old meaning for code in reload which uses this. Bootstrapped and tested on x86_64-linux. Ok? Bernd global-reg-cost.diff PR rtl-optimization/79728 * regs.h (struct target_regs): New field x_contains_allocatable_regs_of_mode. (contains_allocatable_regs_of_mode): New macro. * reginfo.c (init_reg_sets_1): Initialize it, and change contains_reg_of_mode so it includes global regs as well. * reload.c (push_reload): Use contains_allocatable_regs_of_mode rather thanc ontains_regs_of_mode. PR rtl-optimization/79728 * gcc.target/i386/sse-globalreg.c: New test. And I went ahead and installed on the trunk. jeff
Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)
On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek wrote:
> In this testcase we have
> C c = bar (X{1});
> which store_init_value sees as
> c = TARGET_EXPR .n=(&)->i}>)>
> i.e. we're initializing "c" with a TARGET_EXPR. We call replace_placeholders
> that walks the whole tree to substitute the placeholders. Eventually we find
> the nested but that's for another object, so we
> crash. Seems that we shouldn't have stepped into the second TARGET_EXPR at
> all; it has nothing to with "c", it's bar's argument.
>
> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> placeholders in function arguments to cp_gimplify_init_expr which calls
> replace_placeholders for constructors. Not sure if it's enough to handle
> CALL_EXPRs like this, anything else?
Hmm, we might have a DMI containing a call with an argument referring
to *this, i.e.
struct A
{
int i;
int j = frob (this->i);
};
The TARGET_EXPR seems like a more likely barrier, but even there we
could have something like
struct A { int i; };
struct B
{
int i;
A a = A{this->i};
};
I think we need replace_placeholders to keep a stack of objects, so
that when we see a TARGET_EXPR we add it to the stack and therefore
can properly replace a PLACEHOLDER_EXPR of its type.
Jason
Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)
On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill wrote:
> On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek wrote:
>> In this testcase we have
>> C c = bar (X{1});
>> which store_init_value sees as
>> c = TARGET_EXPR > .n=(&)->i}>)>
>> i.e. we're initializing "c" with a TARGET_EXPR. We call replace_placeholders
>> that walks the whole tree to substitute the placeholders. Eventually we find
>> the nested but that's for another object, so we
>> crash. Seems that we shouldn't have stepped into the second TARGET_EXPR at
>> all; it has nothing to with "c", it's bar's argument.
>>
>> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
>> placeholders in function arguments to cp_gimplify_init_expr which calls
>> replace_placeholders for constructors. Not sure if it's enough to handle
>> CALL_EXPRs like this, anything else?
>
> Hmm, we might have a DMI containing a call with an argument referring
> to *this, i.e.
>
> struct A
> {
> int i;
> int j = frob (this->i);
> };
>
> The TARGET_EXPR seems like a more likely barrier, but even there we
> could have something like
>
> struct A { int i; };
> struct B
> {
> int i;
> A a = A{this->i};
> };
>
> I think we need replace_placeholders to keep a stack of objects, so
> that when we see a TARGET_EXPR we add it to the stack and therefore
> can properly replace a PLACEHOLDER_EXPR of its type.
Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
it for later when we lower the TARGET_EXPR.
Jason
Re: [PATCH] Various fixes for facets
On 13/03/17 19:35 +, Jonathan Wakely wrote:
This is a series of patches to fix various bugs in the Unicode
character conversion facets.
Ther first patch fixes a silly < versus <= bug that meant that 0x
got written as a surrogate pair instead of as simply 0xff, and an
endianness bug for the internal representation of UTF-16 code units
stored in char32_t or wchar_t values. That's PR 79511.
The second patch fixes some incorrect bitwise operations (because I
confused & and |) and some incorrect limits (because I confused max
and min). That fixes determining the endianness of the external
representation bytes when they start with a Byte OrderMark, and
correctly reports errors on invalid UCS2. It also fixes
wstring_convert so that it reports the number of characters that were
converted prior to an error. That's PR 79980.
The third patch fixes the output of the encoding() and max_length()
member functions on the codecvt facets, because I wasn't correctly
accounting for a BOM or for the differences between UTF-16 and UCS2.
I plan to commit these for all branches, but I'll wait until after GCC
7.1 is released, and fix it for 7.2 instead. These bugs aren't
important enough to rush into trunk now.
One more patch for a problem found by the libc++ testsuite. Now we
pass all the libc++ tests, and we even pass a test that libc++ fails.
With this, I hope our is 100% conforming. Just in time to be
deprecated for C++17 :-)
commit 3118704bc37cd771b9fc5bf83230f38a16a7c5c3
Author: Jonathan Wakely
Date: Tue Mar 14 17:47:12 2017 +
PR libstdc++/80041 fix codecvt_utf16 to use UTF-16 not UTF-8
PR libstdc++/80041
* src/c++11/codecvt.cc (__codecvt_utf16_base::do_out)
(__codecvt_utf16_base::do_in): Convert char arguments to
char16_t to work with UTF-16 instead of UTF-8.
* testsuite/22_locale/codecvt/codecvt_utf16/80041.cc: New test.
diff --git a/libstdc++-v3/src/c++11/codecvt.cc b/libstdc++-v3/src/c++11/codecvt.cc
index 9c91725..ef38267 100644
--- a/libstdc++-v3/src/c++11/codecvt.cc
+++ b/libstdc++-v3/src/c++11/codecvt.cc
@@ -1217,7 +1217,10 @@ do_out(state_type&, const intern_type* __from, const intern_type* __from_end,
extern_type* __to, extern_type* __to_end,
extern_type*& __to_next) const
{
- range to{ __to, __to_end };
+ range to{
+reinterpret_cast(__to),
+reinterpret_cast(__to_end)
+ };
#if __SIZEOF_WCHAR_T__ == 2
range from{
reinterpret_cast(__from),
@@ -1234,7 +1237,7 @@ do_out(state_type&, const intern_type* __from, const intern_type* __from_end,
return codecvt_base::error;
#endif
__from_next = reinterpret_cast(from.next);
- __to_next = to.next;
+ __to_next = reinterpret_cast(to.next);
return res;
}
@@ -1254,7 +1257,10 @@ do_in(state_type&, const extern_type* __from, const extern_type* __from_end,
intern_type* __to, intern_type* __to_end,
intern_type*& __to_next) const
{
- range from{ __from, __from_end };
+ range from{
+reinterpret_cast(__from),
+reinterpret_cast(__from_end)
+ };
#if __SIZEOF_WCHAR_T__ == 2
range to{
reinterpret_cast(__to),
@@ -1270,7 +1276,7 @@ do_in(state_type&, const extern_type* __from, const extern_type* __from_end,
#else
return codecvt_base::error;
#endif
- __from_next = from.next;
+ __from_next = reinterpret_cast(from.next);
__to_next = reinterpret_cast(to.next);
return res;
}
diff --git a/libstdc++-v3/testsuite/22_locale/codecvt/codecvt_utf16/80041.cc b/libstdc++-v3/testsuite/22_locale/codecvt/codecvt_utf16/80041.cc
new file mode 100644
index 000..a78b194
--- /dev/null
+++ b/libstdc++-v3/testsuite/22_locale/codecvt/codecvt_utf16/80041.cc
@@ -0,0 +1,87 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+//
[PATCH doc] use "cannot" consistently
In formal writing it's recommended to prefer the word "cannot"
to the somewhat informal "can't."
The GCC manual uses "cannot" in most places (280 lines) but there
are a few instances of "can't" (33 lines).
The attached patch replaces the informal "can't" with the former
for consistency.
Thanks
Martin
PS I wasted quite a bit of time updating tm.texi. I kept getting
the error below and didn't realize (forgot) that it was asking me
to copy $objdir/gcc/tm.texi to $srcdir/gcc/doc/tm.texi. Can
someone explain why this file requires these special steps when
the rest of the .texi files can be updated directly?
mv tmp2-tm.texi tmp-tm.texi
/bin/sh /src/gcc/svn/gcc/../move-if-change tmp-tm.texi tm.texi
Verify that you have permission to grant a GFDL license for all
new text in tm.texi, then copy it to /src/gcc/svn/gcc/doc/tm.texi.,
Makefile:2437: recipe for target 's-tm-texi' failed
Index: gcc/doc/extend.texi
===
--- gcc/doc/extend.texi (revision 246134)
+++ gcc/doc/extend.texi (working copy)
@@ -7509,7 +7509,7 @@ In this case, GCC does not actually output assembl
function, unless you specify the option @option{-fkeep-inline-functions}.
If there is a nonintegrated call, then the function is compiled to
assembler code as usual. The function must also be compiled as usual if
-the program refers to its address, because that can't be inlined.
+the program refers to its address, because that cannot be inlined.
@opindex Winline
Note that certain usages in a function definition can make it unsuitable
@@ -9176,7 +9176,7 @@ This results in an incomplete type, much like what
@code{struct foo} without describing the elements. A later declaration
that does specify the possible values completes the type.
-You can't allocate variables or storage using the type while it is
+You cannot allocate variables or storage using the type while it is
incomplete. However, you can work with pointers to that type.
This extension may not be very useful, but it makes the handling of
Index: gcc/doc/hostconfig.texi
===
--- gcc/doc/hostconfig.texi (revision 246134)
+++ gcc/doc/hostconfig.texi (working copy)
@@ -22,7 +22,7 @@ header. @xref{System Config}.
@menu
* Host Common:: Things every host probably needs implemented.
-* Filesystem:: Your host can't have the letter `a' in filenames?
+* Filesystem:: Your host cannot have the letter `a' in filenames?
* Host Misc:: Rare configuration options for hosts.
@end menu
Index: gcc/doc/install.texi
===
--- gcc/doc/install.texi (revision 246134)
+++ gcc/doc/install.texi (working copy)
@@ -3633,7 +3633,7 @@ The libffi library haven't been ported to 64-bit H
Refer to @uref{binaries.html,,binaries} for information about obtaining
precompiled GCC binaries for HP-UX@. Precompiled binaries must be obtained
-to build the Ada language as it can't be bootstrapped using C@. Ada is
+to build the Ada language as it cannot be bootstrapped using C@. Ada is
only available for the 32-bit PA-RISC runtime.
Starting with GCC 3.4 an ISO C compiler is required to bootstrap. The
@@ -3713,12 +3713,12 @@ Although the HP and GNU linkers are both supported
HP linker be used for link editing on this target.
At this time, the GNU linker does not support the creation of long
-branch stubs. As a result, it can't successfully link binaries
+branch stubs. As a result, it cannot successfully link binaries
containing branch offsets larger than 8 megabytes. In addition,
there are problems linking shared libraries, linking executables
with @option{-static}, and with dwarf2 unwind and exception support.
It also doesn't provide stubs for internal calls to global functions
-in shared libraries, so these calls can't be overloaded.
+in shared libraries, so these calls cannot be overloaded.
The HP dynamic loader does not support GNU symbol versioning, so symbol
versioning is not supported. It may be necessary to disable symbol
Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 246134)
+++ gcc/doc/invoke.texi (working copy)
@@ -6471,7 +6471,7 @@ different size.
@opindex Winvalid-pch
@opindex Wno-invalid-pch
Warn if a precompiled header (@pxref{Precompiled Headers}) is found in
-the search path but can't be used.
+the search path but cannot be used.
@item -Wlong-long
@opindex Wlong-long
@@ -10733,7 +10733,7 @@ more details. The run-time behavior can be influe
the available options are shown at startup of the instrumented program. See
@url{https://github.com/google/sanitizers/wiki/AddressSanitizerFlags#run-time-flags}
for a list of supported options.
-The option can't be combined with @option{-fsanitize=thread}
+The option cannot be combined with @option{-fsanitize=thread}
and/
Re: [PATCH 1/5] testsuite: attr-alloc_size-11.c (PR79356)
On 03/12/2017 07:32 PM, Martin Sebor wrote:
On 03/10/2017 10:51 PM, Jeff Law wrote:
On 03/10/2017 09:20 AM, Martin Sebor wrote:
On 03/10/2017 05:57 AM, Rainer Orth wrote:
Hi Segher,
On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote:
Segher Boessenkool writes:
As stated in the PR, this test now passes on aarch64, ia64, powerpc,
and s390x. This patch disables the xfails for those targets.
As I'd mentioned in PR tree-optimization/78775, the test XPASSes on
SPARC, too.
Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk?
[...]
2017-02-09 Segher Boessenkool
gcc/testsuite/
PR testsuite/79356
* gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64,
powerpc,
or s390x.
TBH, I'd strongly prefer to have a proper analysis instead of just
un-xfail-ing the test on an ever growing apparently random list of
targets.
Yeah, I agree. I thought it was just another test that stopped
failing,
but it seems to fail in two ways now, making the testcase succeed?
Lovely. Please consider this patch withdrawn.
I just noticed that nothing has happened at all in a month, so anything
is better than the tests XPASSing on a number of targets.
So the patch is ok for mainline with sparc*-*-* added to the target
lists and a reference to PR testsuite/79356 in the comment.
I'd still be very grateful if Martin could have a look what's really
going on here, though.
Sorry, I haven't had a chance to look more deeply into why these
assertions pass on some targets and fail on others. There is at
least one bug that tracks a VRP problem that manifests only on
some targets and not others (79054). I don't know if this is
a symptom of the same bug or something different. I'll see if
I can find some time to isolate it.
It could well be a BRANCH_COST effect. BRANCH_COST is probably the most
annoying target property that bleeds into the tree/gimple world. From
looking at the gimple in the BZ that could well be the case.
See logical_op_short_circuit for how this is often dealt with in the
testsuite.
Thanks for the hint. I extracted the test case from
attr-alloc_size-11.c and reproduced the discrepancy on powerpc64le
and x86_64. It looks to me like two bugs(?) conspire to trigger
it. I opened pr80006 with the details and I'm working on a patch.
Here's some more detail. First, on x86_64 (but not on powerpc64le
and I suspect the other targets where this assertion passes), GCC
very early on introduces a spurious conversion from signed char to
int. For instance, given:
void* f (signed char x)
{
extern void* ff (signed char) __attribute__ ((alloc_size (1)));
if (-3 <= x && x <= 7)
x = -4;
return ff (x);
}
the tree-original dump on x86_64 is as follows (note the (int)x
cast):
;; Function f (null)
;; enabled by -tree-original
{
extern void * ff (signed char);
if ((unsigned char) x + 3 <= 10)
{
x = -4;
}
return ff ((int) x); <<< spurious cast
}
while the following the same dump on powerpc64le (no cast):
;; Function f (null)
;; enabled by -tree-original
{
extern void * ff (signed char);
if ((unsigned char) x + 3 <= 10)
{
x = -4;
}
return ff (x); <<< no cast
}
Those differences could well be related to promotions implied by the ABI
in use on each target. Those certainly happen too, they're just not as
common a problem as the logical_op_short_circuit stuff.
The cast makes its way to VRP where it causes the range on x
to be lost in the conversion to the temporary it introduces:
Found new range for x_4: ~[-3, 7]
marking stmt to be not simulated again
Visiting statement:
_9 = (int) x_4;
Meeting
[-128, -4]
and
[8, 127]
to
[-128, 127]
Found new range for _9: [-128, 127] <<< anti-range lost
Seems like a problem. Given that we're converting to a wider type, we
ought to be able to do better and not lose the anti range.
...
Visiting PHI node: prephitmp_10 = PHI <_9(3), -4(2)>
...
Meeting
[-128, 127]
and
[-4, -4]
to
[-128, 127]
Intersecting
[-128, 127]
and
[-128, 127]
to
[-128, 127]
Found new range for prephitmp_10: [-128, 127]
...
Visiting statement:
_8 = ff (prephitmp_10);
ISTM that ~[-3,7] U -4 should result in ~[-3,7], right? The
union/intersection of ANTI ranges looked like it could use some
improvement when I was in there last month, so I wouldn't be surprised
if it needs some improvement.
Debugging tree-vrp.c suggests that this is a deficiency in
the extract_range_from_unary_expr function the computes the
range of the result of the NOP_EXPR (the cast) of an SSA_NAME
with an anti-range ~[A, B] as a union of [TYPE_MIN, A - 1] and
[B + 1, TYPE_MAX], which for ~[-3, 7] and the signed char x in
the test case results in [SCHAR_MIN, -4] U [8, SCHAR_MAX], or
[SCHAR_MIN, SCHAR_MAX]. There's a comment in the function
that reads:
/* Now canonicalize anti-ranges to ranges when
Re: [PATCH doc] use "cannot" consistently
Martin Sebor writes: > @@ -373,7 +373,7 @@ example, this code would produce an error: > > @smallexample > #if 0 > -You can't expect this to work. > +You cannot expect this to work. > #endif > @end smallexample > Sure the maintainers would have caught this, but: the "'" is needed here. The point is that code doesn't tokenise properly. Thanks, Richard
Re: [wwwdocs] Document in changes.html -fcode-hoisting, -fipa-bit-cp, -fipa-vrp, -fsplit-loops, GCJ removal, x86 ISA additions, -fshrink-wrap-separate etc.
On 03/12/2017 05:23 PM, Gerald Pfeifer wrote:
Hi Martin,
On Mon, 27 Feb 2017, Martin Sebor wrote:
Sorry to be jumping in so late. I only noticed this bit now.
I would suggest to say that these new built-ins evaluate to integer
constant expressions when their arguments do. Not all C programmers
are familiar with C++ constexpr so they may not understand the use
use case. The request for these built-ins also came from a C user
and was specifically motivated by avoiding -Woverflow warnings so
adding an example demonstrating that might be helpful as well.
Something like this:
... Calls to these built-ins with integer constant arguments
evaluate to integer constants expressions.
For example, in the following, c is assigned
the result of a * b only if the multiplication
does not overflow, otherwise it is assigned the value zero.
The multiplication is performed at compile-time and without
triggering a -Woverflow warning.
enum {
a = 12345678,
b = 87654321,
c = __builtin_mul_overflow_p (a, b, a) ? 0 : a * b
};
this works for me, modulo the closing which I believe will be
necessary. Were you seeing approval for this from someone? (If so,
that may not have been me. ;-)
Thanks. I just committed it.
Martin
[patch, fortran] PR39239 Warning about EQUIVALENCE and VOLATILE
Hello everyone,
a simple patch to throw a warning if not all and not none of the
equivalence objects are volatile. (And the according modification of
gfortran.dg/volatile11.f90)
Nicolas
Regression tested for:
GNU Fortran (GCC) 7.0.1 20170311 (experimental)
Changelog:
2017-03-13 Nicolas Koenig
PR fortran/39239
* resolve.c (resolve_equivalence): Warn if not either
none or all equivalence objects are volatile
* gfortran.dg/volatile11.f90: Changed test to test for
the new warning
Index: fortran/resolve.c
===
--- fortran/resolve.c (revision 246070)
+++ fortran/resolve.c (working copy)
@@ -15560,7 +15560,7 @@ resolve_equivalence (gfc_equiv *eq)
locus *last_where = NULL;
seq_type eq_type, last_eq_type;
gfc_typespec *last_ts;
- int object, cnt_protected;
+ int object, cnt_protected, cnt_volatile;
const char *msg;
last_ts = &eq->expr->symtree->n.sym->ts;
@@ -15569,6 +15569,8 @@ resolve_equivalence (gfc_equiv *eq)
cnt_protected = 0;
+ cnt_volatile = 0;
+
for (object = 1; eq; eq = eq->eq, object++)
{
e = eq->expr;
@@ -15641,6 +15643,17 @@ resolve_equivalence (gfc_equiv *eq)
sym = e->symtree->n.sym;
+ if(sym->attr.volatile_)
+cnt_volatile++;
+ if(cnt_volatile > 0 && cnt_volatile != object)
+ {
+ gfc_warning (0, "Either all or none of the objects in "
+ "the EQUIVALENCE set at %L shall have the "
+ "VOLATILE attribute",
+ &e->where);
+ break;
+ }
+
if (sym->attr.is_protected)
cnt_protected++;
if (cnt_protected > 0 && cnt_protected != object)
Index: testsuite/gfortran.dg/volatile11.f90
===
--- testsuite/gfortran.dg/volatile11.f90 (revision 246070)
+++ testsuite/gfortran.dg/volatile11.f90 (working copy)
@@ -1,8 +1,9 @@
! { dg-do compile }
-! { dg-options "-O2 -fdump-tree-optimized" }
+! { dg-options "-Wall -O2 -fdump-tree-optimized" }
! Tests that volatile can be applied to members of common blocks or
! equivalence groups (PR fortran/35037)
!
+
subroutine wait1
logical event
volatile event
@@ -14,26 +15,10 @@ subroutine wait1
end subroutine
subroutine wait2
- logical event, foo
- volatile event
- equivalence (event, foo)
- event = .false.
- do
-if (event) print *, 'NotOptimizedAway2'
- end do
-end subroutine
-
-subroutine wait3
logical event
integer foo
volatile foo
- equivalence (event, foo)
- event = .false.
- do
-if (event) print *, 'IsOptimizedAway'
- end do
+ equivalence (event, foo) ! { dg-warning "in the EQUIVALENCE set" }
end subroutine
! { dg-final { scan-tree-dump "NotOptimizedAway1" "optimized" } } */
-! { dg-final { scan-tree-dump "NotOptimizedAway2" "optimized" } } */
-! { dg-final { scan-tree-dump-not "IsOptimizedAway" "optimized" } } */
[wwwdocs] remove power.org links from readings.html
Segher confirmed that power.org is gone and that it's okay to remove those links for now. He kindly agreed to help and look for alternatives. Applied. Gerald Index: readings.html === RCS file: /cvs/gcc/wwwdocs/htdocs/readings.html,v retrieving revision 1.269 diff -u -r1.269 readings.html --- readings.html 1 Mar 2017 13:38:38 - 1.269 +++ readings.html 14 Mar 2017 20:17:36 - @@ -252,10 +252,8 @@ rs6000 (powerpc, powerpcle) Manufacturer: IBM, Motorola https://members.openpowerfoundation.org/document/dl/576";>64-Bit ELF V2 ABI - OpenPOWER ABI - https://www.power.org/wp-content/uploads/2013/05/PowerISA_V2.07_PUBLIC.pdf";>POWER8 ISA http://publib16.boulder.ibm.com/pseries/en_US/infocenter/base/43_docs/aixassem/alangref/toc.htm";>AIX V4.3 Assembler Language Ref. http://publibn.boulder.ibm.com/doc_link/en_US/a_doc_lib/aixassem/alangref/alangreftfrm.htm";>AIX 5L Assembler Language Ref. - https://www.power.org/documentation/";>Documentation and tools at power.org rx
Re: [PATCH] Windows support for std::filesystem
niXman 2017-02-12 20:28:
Hi,
Tested on i686/x86_64-MinGW-W64
Please test possible regressions on posix platform.
As continuation for:
https://gcc.gnu.org/ml/libstdc++/2017-02/msg00041.html
Regression on posix platform was fixed.
Tested on i686/x86_64-MinGW-W64 and x86_64-linux-gnu.
Index: libstdc++-v3/src/filesystem/dir.cc
===
--- libstdc++-v3/src/filesystem/dir.cc (revision 246136)
+++ libstdc++-v3/src/filesystem/dir.cc (working copy)
@@ -41,9 +41,10 @@
#endif
#ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS
-# undef opendir
-# define opendir _wopendir
-#endif
+# include "fs-win32.h"
+#else
+# include "fs-posix.h"
+#endif // _GLIBCXX_FILESYSTEM_IS_WINDOWS
namespace fs = std::experimental::filesystem;
@@ -51,7 +52,7 @@
{
_Dir() : dirp(nullptr) { }
- _Dir(DIR* dirp, const fs::path& path) : dirp(dirp), path(path) { }
+ _Dir(os_DIR_t* dirp, const fs::path& path) : dirp(dirp), path(path) { }
_Dir(_Dir&& d)
: dirp(std::exchange(d.dirp, nullptr)), path(std::move(d.path)),
@@ -60,11 +61,11 @@
_Dir& operator=(_Dir&&) = delete;
- ~_Dir() { if (dirp) ::closedir(dirp); }
+ ~_Dir() { if (dirp) ::os_closedir(dirp); }
bool advance(std::error_code*, directory_options = directory_options::none);
- DIR* dirp;
+ os_DIR_t* dirp;
fs::path path;
directory_entry entry;
file_type type = file_type::none;
@@ -87,7 +88,7 @@
if (ec)
ec->clear();
-if (DIR* dirp = ::opendir(p.c_str()))
+if (os_DIR_t* dirp = ::os_opendir(p.c_str()))
return {dirp, p};
const int err = errno;
@@ -105,7 +106,7 @@
}
inline fs::file_type
- get_file_type(const ::dirent& d __attribute__((__unused__)))
+ get_file_type(const ::os_dirent_t& d __attribute__((__unused__)))
{
#ifdef _GLIBCXX_HAVE_STRUCT_DIRENT_D_TYPE
switch (d.d_type)
@@ -145,13 +146,14 @@
ec->clear();
int err = std::exchange(errno, 0);
- const auto entp = readdir(dirp);
+ const auto entp = ::os_readdir(dirp);
std::swap(errno, err);
if (entp)
{
// skip past dot and dot-dot
- if (!strcmp(entp->d_name, ".") || !strcmp(entp->d_name, ".."))
+ if (!std::char_traits::compare(entp->d_name, _WS("."), 1) ||
+ !std::char_traits::compare(entp->d_name, _WS(".."), 2))
return advance(ec, options);
entry = fs::directory_entry{path / entp->d_name};
type = get_file_type(*entp);
@@ -239,7 +241,7 @@
error_code* ec)
: _M_options(options), _M_pending(true)
{
- if (DIR* dirp = ::opendir(p.c_str()))
+ if (os_DIR_t* dirp = ::os_opendir(p.c_str()))
{
auto sp = std::make_shared<_Dir_stack>();
sp->push(_Dir{ dirp, p });
Index: libstdc++-v3/src/filesystem/fs-posix.h
===
--- libstdc++-v3/src/filesystem/fs-posix.h (revision 0)
+++ libstdc++-v3/src/filesystem/fs-posix.h (working copy)
@@ -0,0 +1,49 @@
+
+// Copyright (C) 2014-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// Under Section 7 of GPL version 3, you are granted additional
+// permissions described in the GCC Runtime Library Exception, version
+// 3.1, as published by the Free Software Foundation.
+
+// You should have received a copy of the GNU General Public License and
+// a copy of the GCC Runtime Library Exception along with this program;
+// see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
+//
Re: [PATCH doc] use "cannot" consistently
> The GCC manual uses "cannot" in most places (280 lines) but there > are a few instances of "can't" (33 lines). > > The attached patch replaces the informal "can't" with the former > for consistency. In my opinion, this is the wrong direction. Contractions are becoming more acceptable in even more formal writing and there's even a current US Supreme Court justice who uses them in her opinions. I certainly don't think it's worth a change to use "can't" throughout, but I'm not in favor of eliminating it either.
[PATCH], PR target/79947, Fix compiler segmentation fault with -Ofast -mno-powerpc-gfxopt
In PR target/79947, the code for using the float reciprocal square root
estimate instruction did not check if the -mpowerpc-gfxopt option was enabled.
The code needs this option to generate a conditional floating point move.
I have checked this patch on the trunk, and it fixes the problem and it does
not cause any regressions. Is it ok to apply to the trunk? Assuming there are
no additional problems, can I apply the patch to the GCC 5 and 6 branches as
well?
[gcc]
2017-03-14 Michael Meissner
PR target/79947
* config/rs6000/rs6000.h (TARGET_FRSQRTES): Add check for
-mpowerpc-gfxopt.
[gcc/testsuite]
2017-03-14 Michael Meissner
PR target/79947
* gcc.target/powerpc/pr79947.c: New test.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.h
===
--- gcc/config/rs6000/rs6000.h (revision 246137)
+++ gcc/config/rs6000/rs6000.h (working copy)
@@ -752,7 +752,8 @@ extern int rs6000_vector_align[];
&& (TARGET_POPCNTB || VECTOR_UNIT_VSX_P (DFmode)))
#define TARGET_FRSQRTES(TARGET_HARD_FLOAT && TARGET_POPCNTB \
-&& TARGET_FPRS && TARGET_SINGLE_FLOAT)
+&& TARGET_PPC_GFXOPT && TARGET_FPRS \
+&& TARGET_SINGLE_FLOAT)
#define TARGET_FRSQRTE (TARGET_HARD_FLOAT && TARGET_FPRS \
&& TARGET_DOUBLE_FLOAT \
Index: gcc/testsuite/gcc.target/powerpc/pr79947.c
===
--- gcc/testsuite/gcc.target/powerpc/pr79947.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr79947.c (working copy)
@@ -0,0 +1,12 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-options "-Ofast -mno-powerpc-gfxopt -mcmpb -mno-vsx" } */
+
+/* PR 79949: Compiler segmentation fault due to not having conditional move
+ support for the target if the -mno-powerpc-gfxopt option is used. */
+
+float a, b;
+void
+c ()
+{
+ a = __builtin_sqrtf (b);
+}
[PATCH,RS6000] PR79963: Correct which condition code bit represents result of vec_any_eq built-in function
This patch corrects several errors in a patch that was submitted on 2017-03-01. A copy-and-paste error in the previous patch resulted in accidental use of the lt flag instead of the eq flag to represent the outcome of the vec_any_eq built-in function. Also, in reviewing the code of the previous patch, it was discovered that changes to the C++ templates representing the vec_all_ne and vec_any_eq built-in functions were incomplete. This patch has bootstrapped and been tested on powerpc64le-unknown-linux with no regressions. Is this ok for trunk? gcc/ChangeLog: 2017-03-14 Kelvin Nilsen PR target/79963 * config/rs6000/altivec.h (vec_all_ne): Under __cplusplus++ and __POWER9_VECTOR__ #ifdef control, change template definition to use Power9-specific built-in function. (vec_any_eq): Likewise. * config/rs6000/vector.md (vector_ae_v2di_p): Change the flag used to control outcomes from this test. (vector_ae_p): For VEC_F modes, likewise. Index: gcc/config/rs6000/altivec.h === --- gcc/config/rs6000/altivec.h (revision 246096) +++ gcc/config/rs6000/altivec.h (working copy) @@ -521,9 +521,9 @@ __altivec_scalar_pred(vec_all_nez, __altivec_scalar_pred(vec_any_eqz, __builtin_vec_vcmpnez_p (__CR6_LT_REV, a1, a2)) __altivec_scalar_pred(vec_all_ne, - __builtin_vec_allne_p (a1, a2)) + __builtin_vec_vcmpne_p (a1, a2)) __altivec_scalar_pred(vec_any_eq, - __builtin_vec_anyeq_p (a1, a2)) + __builtin_vec_vcmpae_p (a1, a2)) #endif __altivec_scalar_pred(vec_any_ne, Index: gcc/config/rs6000/vector.md === --- gcc/config/rs6000/vector.md (revision 246096) +++ gcc/config/rs6000/vector.md (working copy) @@ -790,7 +790,7 @@ (eq:V2DI (match_dup 1) (match_dup 2)))]) (set (match_operand:SI 0 "register_operand" "=r") - (lt:SI (reg:CC CR6_REGNO) + (eq:SI (reg:CC CR6_REGNO) (const_int 0))) (set (match_dup 0) (xor:SI (match_dup 0) @@ -837,7 +837,7 @@ (eq:VEC_F (match_dup 1) (match_dup 2)))]) (set (match_operand:SI 0 "register_operand" "=r") - (lt:SI (reg:CC CR6_REGNO) + (eq:SI (reg:CC CR6_REGNO) (const_int 0))) (set (match_dup 0) (xor:SI (match_dup 0)
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 11:07 AM, Bill Schmidt
> wrote:
>>
>> Your suggestion failed bootstrap in libiberty on vprintf-support.c.
>> Compilation failed with:
>>
>> /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc
>> -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/
>> -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/
>>
>> -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/
>>
>> -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/
>> -isystem
>> /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include
>> -isystem
>> /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include
>> -c -DHAVE_CONFIG_H -g -O2 -gtoggle -I.
>> -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include -W -Wall
>> -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic
>> -D_GNU_SOURCE -fPIC
>> /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o
>> pic/vprintf-support.o
>>
>> The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)).
>> Gimplification
>> turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test
>> fails on that.
>
> Reduced test case:
>
> typedef __builtin_va_list __gnuc_va_list;
> typedef __gnuc_va_list va_list;
>
> void
> foo (va_list args)
> {
> va_list ap;
> __builtin_va_copy (ap, args);
> (void)__builtin_va_arg (ap, int);
> __builtin_va_end(ap);
> }
>
Perhaps something like this? It appears to be doing better on bootstrap, and
avoids
failure on both the original problem and the new test case above:
Index: gcc/tree-stdarg.c
===
--- gcc/tree-stdarg.c (revision 246109)
+++ gcc/tree-stdarg.c (working copy)
@@ -1058,7 +1058,13 @@ expand_ifn_va_arg_1 (function *fun)
gimplify_assign (lhs, expr, &pre);
}
else
- gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
+ {
+ enum gimplify_status status;
+ status = gimplify_expr (&expr, &pre, &post, is_gimple_lvalue,
+ fb_lvalue | fb_mayfail);
+ if (status == GS_ERROR)
+ gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue);
+ }
input_location = saved_location;
pop_gimplify_context (NULL);
Bill
Re: [patch, fortran] PR39239 Warning about EQUIVALENCE and VOLATILE
On 03/14/2017 01:17 PM, Nicolas Koenig wrote: > Hello everyone, > > a simple patch to throw a warning if not all and not none of the equivalence > objects are volatile. (And the according modification of > gfortran.dg/volatile11.f90) > > Nicolas > > Regression tested for: > > GNU Fortran (GCC) 7.0.1 20170311 (experimental) > > Changelog: > > 2017-03-13 Nicolas Koenig > > PR fortran/39239 > * resolve.c (resolve_equivalence): Warn if not either none or > all equivalence objects are volatile > * gfortran.dg/volatile11.f90: Changed test to test for the new > warning > > Hi Nicolas, Thanks for starting in on this. Since this results in a warning, maybe change the wording from 'shall' to should. I did not dig into the Fortran Standards so I assume it need not be an error. Also when you submit a patch, please also let us know what platform you regression tested on, such as x86-64-linux, or Windows, or similar. (You can get the whole string from subdirectory names in build directory. On mine its x86_64-pc-linux-gnu) Sometimes we accidentally break things on different platforms. so this way we can see it tested ok over here and seems to fail over there. Your patch has changed some of the scan dumps and I am wondering if you have deleted something we use to check for? Jerry
Re: [PATCH] correct aligned_alloc argument order (PR 80020)
On 03/13/2017 02:44 AM, Richard Biener wrote: On Mon, Mar 13, 2017 at 2:13 AM, Martin Sebor wrote: r243470 decorates standard allocation functions like alloca and malloc with attribute alloc_size. However, in applying the attribute to aligned_alloc I had overlooked that the size argument is the second one and not the first. That oversight has led to __builtin_object_size() reporting the wrong size for aligned_alloc-allocated objects and, consequently, to checking functions like __memset_chk calling abort for buffer bogus overflows. The attached patch corrects this mistake by decorating the function with the correct alloc_size attribute. Ok. I installed the patch on the trunk jeff
Re: [PATCH] add calls.c to GTFILES in Makefile.in
On 03/07/2017 06:03 PM, Martin Sebor wrote: In bug 79936 - ICE with -Walloc-size-larger-than=32767 the reporter encountered an ICE on x86_64-apple-darwin10.8.0 caused by GCC source file that implements the warning accessing a global tree without having included the file in GTFILES make variable. (Thanks again to David Malcolm who helped me root cause this bug.) The attached patch fixes this by adding calls.c to the variable and by including the requisite generated header, gt-calls.h, at the end of the source. Martin gcc-79936.diff PR c/79936 - [7 Regression] ICE with -Walloc-size-larger-than=32767 gcc/ChangeLog: PR c/79936 * Makefile.in (GTFILES): Add calls.c. * calls.c (must_pass_in_stack_var_size_or_pad): Include "gt-calls.h". I fixed the ChangeLog entry for calls.c and installed this. Thanks, jeff
Re: [PATCH], PR target/79947, Fix compiler segmentation fault with -Ofast -mno-powerpc-gfxopt
Hi Mike, On Tue, Mar 14, 2017 at 05:01:53PM -0400, Michael Meissner wrote: > In PR target/79947, the code for using the float reciprocal square root > estimate instruction did not check if the -mpowerpc-gfxopt option was enabled. > The code needs this option to generate a conditional floating point move. > > I have checked this patch on the trunk, and it fixes the problem and it does > not cause any regressions. Is it ok to apply to the trunk? Assuming there > are > no additional problems, can I apply the patch to the GCC 5 and 6 branches as > well? All CPUs that have the reciprocal estimate insns also have the gfxopt insns (and this will not change), so this is okay (also for the branches). Thanks! Segher
Re: [PATCH] MPX: fix PR middle-end/79753
2017-03-10 16:15 GMT+03:00 Martin Liška : > Hello. > > Currently, __builtin_ia32_bndret is used for all functions that have non-void > return type. I think the right fix is to return bounds just for a bounded > type. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Martin Hi, The fix makes sense and allows you to pass the test but it seems like we still have the issue in inlining which can result in bndret call with wrong arg. Do you avoid all such cases by this fix? Can we still have similar problem if cast function with integer return type to a function with pointer return type and the inline the call (not sure if we can inline such calls)? I think this fix still should go to trunk anyway. Thanks, Ilya
[gomp4] cleanup trans-stmt.h
I noticed that gcc/fortran/trans-stmt.h made a reference to a non-existent trans-openacc.c. Those functions have been placed in trans-openmp.c. This patch has been applied to gomp-4_0-branch to correct that error. Cesar 2017-03-14 Cesar Philippidis gcc/fortran/ * trans-stmt.h: Remove stale reference to trans-openacc.c. diff --git a/gcc/fortran/trans-stmt.h b/gcc/fortran/trans-stmt.h index f9c8e74..6ca0c1b 100644 --- a/gcc/fortran/trans-stmt.h +++ b/gcc/fortran/trans-stmt.h @@ -65,8 +65,6 @@ tree gfc_trans_deallocate_array (tree); /* trans-openmp.c */ tree gfc_trans_omp_directive (gfc_code *); void gfc_trans_omp_declare_simd (gfc_namespace *); - -/* trans-openacc.c */ tree gfc_trans_oacc_directive (gfc_code *); tree gfc_trans_oacc_declare (gfc_namespace *);
Re: [PATCH doc] use "cannot" consistently
On 03/14/2017 01:41 PM, Richard Sandiford wrote: Martin Sebor writes: @@ -373,7 +373,7 @@ example, this code would produce an error: @smallexample #if 0 -You can't expect this to work. +You cannot expect this to work. #endif @end smallexample Sure the maintainers would have caught this, but: the "'" is needed here. The point is that code doesn't tokenise properly. Great catch, thanks! I'll be sure to fix that if the patch is approved. Martin
Re: [PATCH doc] use "cannot" consistently
On Tue, 14 Mar 2017, Martin Sebor wrote: > PS I wasted quite a bit of time updating tm.texi. I kept getting > the error below and didn't realize (forgot) that it was asking me > to copy $objdir/gcc/tm.texi to $srcdir/gcc/doc/tm.texi. Can > someone explain why this file requires these special steps when > the rest of the .texi files can be updated directly? Both a GPL copy and a GFDL copy of the text shared between tm.texi and target.def need to be checked in (hence not just having tm.texi.in but not tm.texi checked in). If existing text that was only in one place (so only under one of GPL and GFDL) is being moved so that there are copies under both licenses, then this involves docstring relicensing review (otherwise, just copying the generated tm.texi without any extra review for that copying is appropriate). -- Joseph S. Myers jos...@codesourcery.com
Re: [patch, fortran] PR39239 Warning about EQUIVALENCE and VOLATILE
On 03/14/2017 10:42 PM, Jerry DeLisle wrote:
On 03/14/2017 01:17 PM, Nicolas Koenig wrote:
Hello everyone,
a simple patch to throw a warning if not all and not none of the equivalence
objects are volatile. (And the according modification of
gfortran.dg/volatile11.f90)
Nicolas
Regression tested for:
GNU Fortran (GCC) 7.0.1 20170311 (experimental)
Changelog:
2017-03-13 Nicolas Koenig
PR fortran/39239
* resolve.c (resolve_equivalence): Warn if not either none or
all equivalence objects are volatile
* gfortran.dg/volatile11.f90: Changed test to test for the new
warning
Hi Nicolas,
Thanks for starting in on this.
Since this results in a warning, maybe change the wording from 'shall' to
should.
I did not dig into the Fortran Standards so I assume it need not be an error.
Also when you submit a patch, please also let us know what platform you
regression tested on, such as x86-64-linux, or Windows, or similar. (You can get
the whole string from subdirectory names in build directory. On mine its
x86_64-pc-linux-gnu) Sometimes we accidentally break things on different
platforms. so this way we can see it tested ok over here and seems to fail over
there.
Your patch has changed some of the scan dumps and I am wondering if you have
deleted something we use to check for?
Jerry
Hello Jerry,
I have to thank for the kind feedback.
Attached is a reworked version of the patch with the changes applied. It
also should have the same scan dump now, one of the test cases was
edited stupidly.
The regression tests for both the old as well as the new test have been
performed on an x86-64-linux (x86_64-pc-linux-gnu).
Nicolas
Index: fortran/resolve.c
===
--- fortran/resolve.c (revision 246143)
+++ fortran/resolve.c (working copy)
@@ -15560,7 +15560,7 @@ resolve_equivalence (gfc_equiv *eq)
locus *last_where = NULL;
seq_type eq_type, last_eq_type;
gfc_typespec *last_ts;
- int object, cnt_protected;
+ int object, cnt_protected, cnt_volatile;
const char *msg;
last_ts = &eq->expr->symtree->n.sym->ts;
@@ -15569,6 +15569,8 @@ resolve_equivalence (gfc_equiv *eq)
cnt_protected = 0;
+ cnt_volatile = 0;
+
for (object = 1; eq; eq = eq->eq, object++)
{
e = eq->expr;
@@ -15641,6 +15643,17 @@ resolve_equivalence (gfc_equiv *eq)
sym = e->symtree->n.sym;
+ if(sym->attr.volatile_)
+cnt_volatile++;
+ if(cnt_volatile > 0 && cnt_volatile != object)
+ {
+ gfc_warning (0, "Either all or none of the objects in "
+ "the EQUIVALENCE set at %L should have the "
+ "VOLATILE attribute",
+ &e->where);
+ break;
+ }
+
if (sym->attr.is_protected)
cnt_protected++;
if (cnt_protected > 0 && cnt_protected != object)
Index: testsuite/gfortran.dg/volatile11.f90
===
--- testsuite/gfortran.dg/volatile11.f90 (revision 246140)
+++ testsuite/gfortran.dg/volatile11.f90 (working copy)
@@ -3,6 +3,7 @@
! Tests that volatile can be applied to members of common blocks or
! equivalence groups (PR fortran/35037)
!
+
subroutine wait1
logical event
volatile event
@@ -16,7 +17,7 @@ end subroutine
subroutine wait2
logical event, foo
volatile event
- equivalence (event, foo)
+ equivalence (event, foo) ! { dg-warning "in the EQUIVALENCE set" }
event = .false.
do
if (event) print *, 'NotOptimizedAway2'
@@ -27,7 +28,7 @@ subroutine wait3
logical event
integer foo
volatile foo
- equivalence (event, foo)
+ equivalence (event, foo) ! { dg-warning "in the EQUIVALENCE set" }
event = .false.
do
if (event) print *, 'IsOptimizedAway'
patch, libfortran] [patch, fortran] Fix PR 79956, part two, attempt 3
Hello world, well, here is the third attempt at fixing the second part of the PR. Glancing over the source, I think there are quite a few places where we currently issue a runtime error which we could replace by an assert, but that's something for 8.0. Regression-tested on x86_64-pc-linux-gnu. OK for trunk? Thomas (I won't be able to commit for a few days, so if somebody wants to commit for me, feel free). 2017-03-12 Thomas Koenig PR libfortran/79956 * libgfortran.h (GFC_ASSERT): New macro. * m4/reshape.m4 (reshape_'rtype_ccode`): Use GFC_ASSERT to specify that sdim > 0 and rdim > 0. * intrinsic/reshape_generic.c (reshape_internal): Likweise. * generated/reshape_c10.c: Regenerated. * generated/reshape_c16.c: Regenerated. * generated/reshape_c4.c: Regenerated. * generated/reshape_c8.c: Regenerated. * generated/reshape_i16.c: Regenerated. * generated/reshape_i4.c: Regenerated. * generated/reshape_i8.c: Regenerated. * generated/reshape_r10.c: Regenerated. * generated/reshape_r16.c: Regenerated. * generated/reshape_r4.c: Regenerated. * generated/reshape_r8.c: Regenerated. Index: generated/reshape_c10.c === --- generated/reshape_c10.c (Revision 245760) +++ generated/reshape_c10.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_c10 (gfc_array_c10 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_c16.c === --- generated/reshape_c16.c (Revision 245760) +++ generated/reshape_c16.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_c16 (gfc_array_c16 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_c4.c === --- generated/reshape_c4.c (Revision 245760) +++ generated/reshape_c4.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_c4 (gfc_array_c4 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_c8.c === --- generated/reshape_c8.c (Revision 245760) +++ generated/reshape_c8.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_c8 (gfc_array_c8 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_i16.c === --- generated/reshape_i16.c (Revision 245760) +++ generated/reshape_i16.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_16 (gfc_array_i16 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_i4.c === --- generated/reshape_i4.c (Revision 245760) +++ generated/reshape_i4.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_4 (gfc_array_i4 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_i8.c === --- generated/reshape_i8.c (Revision 245760) +++ generated/reshape_i8.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_8 (gfc_array_i8 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoids a warning. */ + GFC_ASSERT(sdim>0); + ssize = 1; sempty = 0; for (n = 0; n < sdim; n++) Index: generated/reshape_r10.c === --- generated/reshape_r10.c (Revision 245760) +++ generated/reshape_r10.c (Arbeitskopie) @@ -232,6 +232,11 @@ reshape_r10 (gfc_array_r10 * const restrict ret, } sdim = GFC_DESCRIPTOR_RANK (source); + + /* sdim is always > 0; this lets the compiler optimize more and + avoid
Re: Combiner fix for PR79910
On 03/10/2017 04:24 PM, Bernd Schmidt wrote:
In this PR, we have a few insns involved in two instruction combinations:
insn 16: set r100
insn 27: some calculation
insn 28: some calculation
insn 32: using r100
insn 33: using r100
insn 35: some calculation
Then we combine insns 27, 28 and 33, producing two output insns, As a
result, insn 28 (i2) now obtains a use of r100. But insn 32, which is
not involved in this combination, has the LOG_LINKS entry for that
register, and we don't know that we need to update it. As a result, the
second combination, involving regs 16, 32 and 35 (based on the remaining
LOG_LINK for r100), produces incorrect code, as we don't realize there's
a use of r100 before insn 32.
The following fixes it. Bootstrapped and tested on x86_64-linux, ok (on
all branches)?
Bernd
79910.diff
PR rtl-optimization/79910
* combine.c (record_used_regs): New static function.
(try_combine): Handle situations where there is an additional
instruction between I2 and I3 which needs to have a LOG_LINK
updated.
PR rtl-optimization/79910
* gcc.dg/torture/pr79910.c: New test.
What a nasty little problem.
I don't like that we have to build these bitmaps due to the compile-time
cost. Would it make sense to only build them when i0/i1 exist?
We don't do 4->3 combinations, just 4->2 and 3->2, so it's only the
i2pattern where we might need to conjure up a LOG_LINK, right?
We don't do 4->3 combinations, right? So we only have to care about
when there's going to be an newi2pat, right (3->2 or 4->2).
We don't ever create a newi1pat (for a 4->3 combination), right? So we
only have to worry about testing when there's a newi2pat, right?
Do you need to handle 4->3, in which case the new bits could be on newi1pat?
Index: gcc/combine.c
===
--- gcc/combine.c (revision 245685)
+++ gcc/combine.c (working copy)
@@ -2559,6 +2560,57 @@ can_split_parallel_of_n_reg_sets (rtx_in
return true;
}
@@ -4051,6 +4124,33 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
return 0;
}
+ auto_bitmap new_regs_in_i2;
+ if (newi2pat)
+{
+ /* We need to discover situations where we introduce a use of a
+register into I2, where none of the existing LOG_LINKS contain
+a reference to it. This can happen if previously I3 referenced
+the reg, and there is an additional use between I2 and I3. We
+must remove the LOG_LINKS entry from that additional use and
+distribute it along with our own ones. */
+ note_uses (&newi2pat, record_used_regs, (bitmap)new_regs_in_i2);
+ bitmap_and_compl_into (new_regs_in_i2, i2_regset);
+ bitmap_and_compl_into (new_regs_in_i2, links_regset);
+
+ /* Here, we first look for situations where a hard register use
+ moved, and just give up. This should happen approximately
+ never, and it's not worth it to deal with possibilities like
+ multi-word registers. Later, when fixing up LOG_LINKS, we
+ deal with the case where a pseudo use moved. */
+ if (prev_nonnote_insn (i3) != i2)
+ for (unsigned r = 0; r < FIRST_PSEUDO_REGISTER; r++)
+ if (bitmap_bit_p (new_regs_in_i2, r))
if (prev_nonnote_insn (i3) != i2
&& bitmap_first_set_bit (new_regs_in_i2) < FIRST_PSEUDO_REGISTER)
?
Overall it seems reasonable. If possible, let's avoid the calls to
create the bitmaps in case where we know we'll never be creating a new
use in i2.
I'm not wed to using bitmap_first_set_bit. It's clearer and may be
faster since you're not doing a ton of calls into bitmap_bit_p. The
bitmap_first_set_bit implementation under the hood finds the word within
the first bitmap element that is nonzero, then uses ctzl on that to get
its bit.
Jeff
Re: Combiner fix for PR79910
On 03/15/2017 12:03 AM, Jeff Law wrote: On 03/10/2017 04:24 PM, Bernd Schmidt wrote: PR rtl-optimization/79910 * combine.c (record_used_regs): New static function. (try_combine): Handle situations where there is an additional instruction between I2 and I3 which needs to have a LOG_LINK updated. PR rtl-optimization/79910 * gcc.dg/torture/pr79910.c: New test. What a nasty little problem. I don't like that we have to build these bitmaps due to the compile-time cost. Would it make sense to only build them when i0/i1 exist? I suppose at the moment we don't do 2->2 combinations, so we could conditionalize this on having an i1. We don't do 4->3 combinations, just 4->2 and 3->2, so it's only the i2pattern where we might need to conjure up a LOG_LINK, right? We don't do 4->3 combinations, right? So we only have to care about when there's going to be an newi2pat, right (3->2 or 4->2). We don't ever create a newi1pat (for a 4->3 combination), right? So we only have to worry about testing when there's a newi2pat, right? Yes to all, there isn't a newi1pat, only 4->2 and 3->2 can be an issue. +if (prev_nonnote_insn (i3) != i2) + for (unsigned r = 0; r < FIRST_PSEUDO_REGISTER; r++) +if (bitmap_bit_p (new_regs_in_i2, r)) if (prev_nonnote_insn (i3) != i2 && bitmap_first_set_bit (new_regs_in_i2) < FIRST_PSEUDO_REGISTER) Ah. I had wondered about the loop but only thought in the direction of intersecting this bitmap with one of all hard regs (and I think there isn't such a bitmap, so I kept the loop). I'll retest with your suggestion and with the bitmap creation conditional on i1 being nonnull. Bernd
Re: [PATCH] PR libstdc++/79789 fix non-reserved names in headers
On 10/03/17 15:20 +, Jonathan Wakely wrote: On 09/03/17 19:46 +, Jonathan Wakely wrote: On 03/03/17 10:47 -0500, David Edelsohn wrote: This patch caused a new regression on AIX. I'm unable to bootstrap on either gcc111 or gcc119 so I can't test the fix. export CONFIG_SHELL=/usr/bin/bash PATH=/opt/freeware/bin:$PATH $gccsrcdir/configure ... --disable-werror --with-included-gettext --with-gmp=/opt/cfarm --with-libiconv-prefix=/opt/cfarm --disable-libstdcxx-pch Here's what I'm committing to trunk. Tested powerpc64le-linux and powerpc-ibm-aix7.2.0.0 Also committed to gcc-5-branch and gcc-6-branch. commit 5e390a2874a9629c13eaddb76f82a66f0634a864 Author: Jonathan Wakely Date: Fri Mar 10 13:14:33 2017 + Fix libstdc++ reserved names test to pass on AIX * testsuite/17_intro/names.cc: Undefine macros that clash with identifiers in AIX system headers. diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc index a7d9a6b..c525861 100644 --- a/libstdc++-v3/testsuite/17_intro/names.cc +++ b/libstdc++-v3/testsuite/17_intro/names.cc @@ -98,4 +98,13 @@ #define x ( #define y ( #define z ( + +#ifdef _AIX +// See https://gcc.gnu.org/ml/libstdc++/2017-03/msg00015.html +#undef f +#undef r +#undef x +#undef y +#endif + #include
Reload fix for an old aarch64 issue
This triggered a kernel miscompilation with an old (4.8 I think) aarch64
toolchain.
Here's the reloads for the insn where things go wrong:
Reloads for insn # 210
Reload 0: reload_in (DI) = (reg/v/f:DI 80 [ pgdata ])
GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 0)
reload_in_reg: (reg/v/f:DI 80 [ pgdata ])
reload_reg_rtx: (reg:DI 5 x5)
Reload 1: reload_in (DI) = (plus:DI (reg/v/f:DI 80 [ pgdata ])
(const_int 7172
[0x1c04]))
GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 0), inc by 2
reload_in_reg: (plus:DI (reg/v/f:DI 80 [ pgdata ])
(const_int 7172
[0x1c04]))
reload_reg_rtx: (reg:DI 5 x5)
Note how the input and input_address reloads use the same reload
register. This is intended as the two categories aren't supposed to
conflict. The problem is that reload 1 is a PLUS expression, and when
reloading these, gen_reload may have to resort to tricks, such as
loading one of the operands (the constant 7172) into the reload
register, and then adding the other operand to it. In effect that's an
earlyclobber of the reload register, and that is not represented in the
for_input/for_input_address classification.
Most likely we haven't tripped over this earlier because on other
machines the addition can be done in a single insn.
The following fixes this by using RELOAD_OTHER in this case. This is
pessimistic, but at that point I don't think we can really know what
gen_reload will have to do. Bootstrapped and tested on x86_64-linux on
the 4.7 branch - that's the best method of testing that I can think of
(but I think I'll also run some c6x tests with trunk). If no one
objects, I'll check this in soonish.
Bernd
* reload.c (find_reloads): When reloading a nonoffsettable address,
use RELOAD_OTHER for it and its address reloads.
Index: gcc/reload.c
===
--- gcc/reload.c (revision 211480)
+++ gcc/reload.c (working copy)
@@ -3989,14 +3989,14 @@ find_reloads (rtx insn, int replace, int
&XEXP (recog_data.operand[i], 0), (rtx*) 0,
base_reg_class (VOIDmode, as, MEM, SCRATCH),
address_mode,
- VOIDmode, 0, 0, i, RELOAD_FOR_INPUT);
+ VOIDmode, 0, 0, i, RELOAD_OTHER);
rld[operand_reloadnum[i]].inc
= GET_MODE_SIZE (GET_MODE (recog_data.operand[i]));
/* If this operand is an output, we will have made any
reloads for its address as RELOAD_FOR_OUTPUT_ADDRESS, but
now we are treating part of the operand as an input, so
- we must change these to RELOAD_FOR_INPUT_ADDRESS. */
+ we must change these to RELOAD_FOR_OTHER_ADDRESS. */
if (modified[i] == RELOAD_WRITE)
{
@@ -4005,10 +4005,10 @@ find_reloads (rtx insn, int replace, int
if (rld[j].opnum == i)
{
if (rld[j].when_needed == RELOAD_FOR_OUTPUT_ADDRESS)
- rld[j].when_needed = RELOAD_FOR_INPUT_ADDRESS;
+ rld[j].when_needed = RELOAD_FOR_OTHER_ADDRESS;
else if (rld[j].when_needed
== RELOAD_FOR_OUTADDR_ADDRESS)
- rld[j].when_needed = RELOAD_FOR_INPADDR_ADDRESS;
+ rld[j].when_needed = RELOAD_FOR_OTHER_ADDRESS;
}
}
}
Re: Reload fix for an old aarch64 issue
On Tue, Mar 14, 2017 at 4:22 PM, Bernd Schmidt wrote: > This triggered a kernel miscompilation with an old (4.8 I think) aarch64 > toolchain. Yes RHEL's 4.8 has the issue. So did Cavium/MontaVista's GCC 4.7 with AARCH64 support backported to it. We would work around the bug in the kernel some of the time. I never looked into it since I knew it was in some part of reload and I am not a person to look into a reload issue. Thanks, Andrew > > Here's the reloads for the insn where things go wrong: > > Reloads for insn # 210 > Reload 0: reload_in (DI) = (reg/v/f:DI 80 [ pgdata ]) > GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 0) > reload_in_reg: (reg/v/f:DI 80 [ pgdata ]) > reload_reg_rtx: (reg:DI 5 x5) > Reload 1: reload_in (DI) = (plus:DI (reg/v/f:DI 80 [ pgdata ]) > (const_int 7172 > [0x1c04])) > GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 0), inc by 2 > reload_in_reg: (plus:DI (reg/v/f:DI 80 [ pgdata ]) > (const_int 7172 > [0x1c04])) > reload_reg_rtx: (reg:DI 5 x5) > > Note how the input and input_address reloads use the same reload register. > This is intended as the two categories aren't supposed to conflict. The > problem is that reload 1 is a PLUS expression, and when reloading these, > gen_reload may have to resort to tricks, such as loading one of the operands > (the constant 7172) into the reload register, and then adding the other > operand to it. In effect that's an earlyclobber of the reload register, and > that is not represented in the for_input/for_input_address classification. > > Most likely we haven't tripped over this earlier because on other machines > the addition can be done in a single insn. > > The following fixes this by using RELOAD_OTHER in this case. This is > pessimistic, but at that point I don't think we can really know what > gen_reload will have to do. Bootstrapped and tested on x86_64-linux on the > 4.7 branch - that's the best method of testing that I can think of (but I > think I'll also run some c6x tests with trunk). If no one objects, I'll > check this in soonish. > > > Bernd
