Re: [PATCH, rs6000] PR target/89112 [8/9 Regression] fix bdnzt pattern for long branch case
On Sat, Feb 02, 2019 at 05:17:31PM -0600, Aaron Sawdey wrote:
> I needed to introduce a local label in this pattern because output_cbranch
> put out a second instruction
> in the long branch case. This fixes the issue but there are a couple ways
> this could be improved:
>
> * output_cbranch() is passed the original insn and assumes from that that the
> branch is a long
> branch. However this is incorrect because we are just branching to a local
> label we know is only
> a few instructions away. If there is a way to fix this, an unnecessary branch
> could be eliminated.
Maybe output_cbranch can be made aware of bdz{,t,f} and bdnz{,t,f}?
> * While the long branch case of this pattern needs to work, the real problem
> is that part of
> the code emitted by the memcmp expansion is being treated as cold code and
> moved to the end of
> the function. Ideally all of this code should stay together. I suspect I need
> to make some kind
> of branch frequency notation for this to happen.
You can emit a REG_BR_PROB note on the branches that need one?
> Regstrap passes on ppc64le power7/8/9, ok for trunk and backport to 8?
I pre-approved it in the PR, the messages crossed I think :-)
But, hrm. Labels ".L" are already used for something else, with
something else for . Please use a unique name? Okay with that
change. Thanks!
Segher
> 2019-02-02 Aaron Sawdey
>
> * config/rs6000/rs6000.md (tf_): generate a local label
> for the long branch case.
>
> Index: gcc/config/rs6000/rs6000.md
> ===
> --- gcc/config/rs6000/rs6000.md (revision 268403)
> +++ gcc/config/rs6000/rs6000.md (working copy)
> @@ -12639,8 +12639,8 @@
>else
> {
>static char seq[96];
> - char *bcs = output_cbranch (operands[3], "$+8", 1, insn);
> - sprintf(seq, " $+12\;%s;b %%l0", bcs);
> + char *bcs = output_cbranch (operands[3], ".L%=", 1, insn);
> + sprintf(seq, " .L%%=\;%s\;b %%l0\;.L%%=:", bcs);
>return seq;
> }
> }
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/1/19 4:32 AM, Marc Glisse wrote:
Hello,
first, I expect this to be controversial, so feel free to complain.
I don't feel too strongly about whether -Wmaybe-uninitialized should
be in -Wall or in -Wextra, and I might even be somewhat more inclined
to expect to find it in the latter. But since you sound like you are
gearing up for proposing other changes in the same vein down the line
where I might have stronger opinions, I should comment.
[It's a bit of a long-winded response because I've been thinking about
this topic a lot lately.]
In general, I think a discussion of warning groupings is worth having
(even needed) at the right time, but stage 4 doesn't strike me as
the most opportune occasion.
Specifically for -Wmaybe-uninitialized, the option has been in -Wall
for many releases, and no major changes to it have been made recently
that I know. So what I'm missing in this proposal is: why now? What
has changed to make this a pressing issue now? Has its rate of false
positives gone up significantly? If so, by how much and why?
The description of -Wall says "This enables all the warnings about
constructions that some users consider questionable, and that are easy
to avoid (or modify to prevent the warning), even in conjunction with
macros."
And the description of -Wmaybe-uninitialized "For an automatic variable,
if there exists a path from the function entry to a use of the variable
that is initialized, but there exist some other paths for which the
variable is not initialized, the compiler emits a warning if it cannot
prove the uninitialized paths are not executed at run time. These
warnings are made optional because GCC is not smart enough to see all
the reasons why the code might be correct in spite of appearing to have
an error."
-Wmaybe-uninitialized generates false positives, we can tweak the
compiler to reduce them, but there will always be some, that's in the
nature of this warning.
Please be clear about what you mean by false positives. Is it that
the warning triggers contrary to the documentation ("a path exists
where the variable is uninitialized along with one where it is"),
or that the path to the use of the variable does exist but we
(though not GCC) can tell from the context that it cannot be reached?
(The latter wouldn't qualify as a false positive as the term is defined
in literature; i.e., the warning works as designed, we just don't like
or agree with it.)
In practice, false positives (and negatives) of both kinds, whether
they fit the formal definition or the informal one, are the nature
of virtually all non-trivial static diagnostics, certainly all those
that depend on control or data flow analysis. Some are due to bugs
or limitations in the implementation of the warning. Others are
inherent in the technology. Is this warning more prone to one kind
than others? If so, is it because it's implemented poorly, or that
its implementation hasn't kept up with the improvements to
the optimizers, or has the infrastructure it depends on become
ill-suited for it to avoid some of the false positives (as formally
defined), or is it something else?
These false positives are not easy to avoid, as required to be part of
-Wall. Avoiding them, when it is possible at all, requires not just a
syntactic tweak, like adding parentheses, but a semantic change that can
make the code worse. Initializing something that does not need it is
extra code (increases code size and running time). It also prevents
better tools from detecting true uninitialized uses, either static
analyzers or runtime checkers (sanitizer, valgrind).
I don't find the argument very compelling that diagnosing potential
bugs should be avoided because the fix (or the suppression in
the case of a false positive) could be wrong. The risk exists with
all diagnostics.
I also take issue with the suggestion that dynamic analysis is
necessarily a superior mechanism for detecting bugs. They each have
their strengths and weaknesses. They are not an either-or proposition
but rather complementary solutions.
Lastly, in the case of uninitialized variables, the usual solution
of initializing them is trivial and always safe (some coding styles
even require it). Initializing scalars has a negligible performance
impact in practice, and, if it's thought to be necessary, can easily
be done conditionally (as in, based on some macro) so that
the uninitialized accesses can still be detected by dynamic analysis.
This message concentrates on the negatives, but that doesn't mean I
consider -Wmaybe-uninitialized as useless. It can find true
uninitialized uses. And even some false positives can point at places
where we can help the compiler generate better code (say with a default
__builtin_unreachable case in a switch). I did in the past contribute
patches to make it warn more often, and I might do so again in the future.
This paragraph makes me think you equate false positives from this
warning with those from
[PATCH, rs6000] PR target/89112 [8/9 Regression] fix bdnzt pattern for long branch case
I needed to introduce a local label in this pattern because output_cbranch put
out a second instruction
in the long branch case. This fixes the issue but there are a couple ways this
could be improved:
* output_cbranch() is passed the original insn and assumes from that that the
branch is a long
branch. However this is incorrect because we are just branching to a local
label we know is only
a few instructions away. If there is a way to fix this, an unnecessary branch
could be eliminated.
* While the long branch case of this pattern needs to work, the real problem is
that part of
the code emitted by the memcmp expansion is being treated as cold code and
moved to the end of
the function. Ideally all of this code should stay together. I suspect I need
to make some kind
of branch frequency notation for this to happen.
Regstrap passes on ppc64le power7/8/9, ok for trunk and backport to 8?
Thanks!
2019-02-02 Aaron Sawdey
* config/rs6000/rs6000.md (tf_): generate a local label
for the long branch case.
Index: gcc/config/rs6000/rs6000.md
===
--- gcc/config/rs6000/rs6000.md (revision 268403)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -12639,8 +12639,8 @@
else
{
static char seq[96];
- char *bcs = output_cbranch (operands[3], "$+8", 1, insn);
- sprintf(seq, " $+12\;%s;b %%l0", bcs);
+ char *bcs = output_cbranch (operands[3], ".L%=", 1, insn);
+ sprintf(seq, " .L%%=\;%s\;b %%l0\;.L%%=:", bcs);
return seq;
}
}
--
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
Re: [PATCH, rs6000] Correct dg directives on recently added vec-extract tests
Hi! On Fri, Feb 01, 2019 at 02:58:48PM -0600, Kelvin Nilsen wrote: > Overnight regression testing revealed a portability problem with several > recently installed tests. The tests were observed to fail on a power7 test > platform. > > The tests, which are intended to execute, are compiled with -mcpu=power8. > Thus, they require power 8 hardware. > > I have regression tested this on powerpc64-linux (P7 big-endian, both -m32 > and -m64), both 32-bit and 64-bit. Is this ok for trunk and for various > backports to which the original patch is to be directed? Okay for both trunk and relevant backports. Thanks! Segher > 2019-02-01 Kelvin Nilsen > > * gcc.target/powerpc/vec-extract-slong-1.c: Require p8 execution > hardware. > * gcc.target/powerpc/vec-extract-schar-1.c: Likewise. > * gcc.target/powerpc/vec-extract-sint128-1.c: Likewise. > * gcc.target/powerpc/vec-extract-sshort-1.c: Likewise. > * gcc.target/powerpc/vec-extract-ulong-1.c: Likewise. > * gcc.target/powerpc/vec-extract-uchar-1.c: Likewise. > * gcc.target/powerpc/vec-extract-sint-1.c: Likewise. > * gcc.target/powerpc/vec-extract-uint128-1.c: Likewise. > * gcc.target/powerpc/vec-extract-ushort-1.c: Likewise. > * gcc.target/powerpc/vec-extract-uint-1.c: Likewise.
Re: [PATCH fortran] PR 81344 - Can't disable -ffpe-trap (or not documented)
Committed as revision r268480 after approval by Jerry on IRC. Cheers, Dominique
Re: [libphobos] Work around lack of dlpi_tls_modid before Solaris 11.5
Hi Rainer, I suspect the two testsuite regressions (compared to a build with dlpi_tls_modid present) I mentioned are exactly of the kind you mention: e.g. the gdc.test/runnable/testaa.d failures are like this core.exception.rangeer...@gdc.test/runnable/testaa.d(410): Range violation /vol/gcc/src/hg/trunk/local/libphobos/libdruntime/core/exception.d:496 onRangeError [0x80f0d2c] /vol/gcc/src/hg/trunk/local/libphobos/libdruntime/core/exception.d:672 _d_arraybounds [0x80f132f] ??:? void testaa.test15() [0x80d7ae4] ??:? _Dmain [0x80dd3fc] before test 1 and gdc.test/runnable/xtest55.d fails like so: core.exception.asserter...@gdc.test/runnable/xtest55.d(19): Assertion failure /vol/gcc/src/hg/trunk/local/libphobos/libdruntime/core/exception.d:441 onAssertError [0x7fff55dd3b56] ??:? _Dmain [0x418959] 7FFFBEB07FFFBEB0 If you want to verify whether it's really a GC problem, you can add this in the main function to disable GC collections: import core.memory; GC.disable(); This should be fine for the test suite. If you want to do this for the unit tests it's slightly more complicated as the main functions is executed _after_ all unit tests. IIRC adding it in a shared static this() module constructor would work there. Best regards, Johannes
Re: Move -Wmaybe-uninitialized to -Wextra
On Fri, Feb 01, 2019 at 12:27:57PM -0700, Jeff Law wrote: > On 2/1/19 7:01 AM, Marek Polacek wrote: > > On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: > >> On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > >>> My opinion is that -Wmaybe-uninitialized would serve its purpose better > >>> as > >>> part of -Wextra. > >> > >> +1 > > > > +1 from me too. > I disagree strongly. If we move it to Wextra it's going to see a lot > less usage in real world codebases and potentially lead to the > re-introduction of a class of bugs that we've largely helped stomp out. The usual workaround, especially for programs that build with multiple (i.e. older) versions of GCC, is to initialise any such variable (to an either or not useful value) early. This doesn't fix the actual problem usually (which is that your control flow is too complex). > It's also the case that maybe uninitialized vs is uninitialized is > really just a function of CFG shape. Give me any "maybe uninitialized" > case and I can turn it into a "is uninitialized" with simple block > duplication of the forms done by jump threading, path isolation, > superblock formation, etc. Are you saying that -Wmaybe-uninitialized should be included in -Wuninitialized, since it has no extra false positives? That wasn't true at all historically: -Wuninitialized has false positives on paths that cannot be executed because of function preconditions, but -Wmaybe-uninitialized used to warn for things that can be locally proven to never execute, like if (a) b = 42; ... if (a) f(b); > >> Yes, using -Werror is usually a terrible idea. > Generally agreed in released versions of any code. -Werror *may* be > appropriate in development versions depending on the project's policies, > procedures and quality of codebase. IMO it is more useful it is much more useful if you make your build system less noisy so that problems are more obvious, instead of breaking the build for no reason all the time (see PR89162, etc. etc.) Segher
Re: [PATCH 00/46] Implement MMX intrinsics with SSE
On Sat, Feb 2, 2019 at 9:07 AM Florian Weimer wrote: > > * H. J. Lu: > > > 1. MMX maskmovq and SSE2 maskmovdqu aren't equivalent. We emulate MMX > > maskmovq with SSE2 maskmovdqu by zeroing out the upper 64 bits of the > > mask operand. A warning is issued since invalid memory access may > > happen when bits 64:127 at memory location are unmapped: > > > > xmmintrin.h:1168:3: note: Emulate MMX maskmovq with SSE2 maskmovdqu may > > result i > > n invalid memory access > > 1168 | __builtin_ia32_maskmovq ((__v8qi)__A, (__v8qi)__N, __P); > > | ^~~ > > Would it be possible to shift the mask according to the misalignment in > the address? I think this should allow avoiding crossing a page > boundary if the orginal 64-bit load would not. I guess it is possible. But it may be quite a bit complex for for no apparent gains since we also need to shift the implicit memory address. -- H.J.
Re: [PATCH 00/46] Implement MMX intrinsics with SSE
* H. J. Lu: > 1. MMX maskmovq and SSE2 maskmovdqu aren't equivalent. We emulate MMX > maskmovq with SSE2 maskmovdqu by zeroing out the upper 64 bits of the > mask operand. A warning is issued since invalid memory access may > happen when bits 64:127 at memory location are unmapped: > > xmmintrin.h:1168:3: note: Emulate MMX maskmovq with SSE2 maskmovdqu may > result i > n invalid memory access > 1168 | __builtin_ia32_maskmovq ((__v8qi)__A, (__v8qi)__N, __P); > | ^~~ Would it be possible to shift the mask according to the misalignment in the address? I think this should allow avoiding crossing a page boundary if the orginal 64-bit load would not.
Re: [patch, fortran] Fix PR 88298
OK - thanks for the patch. Paul On Sat, 2 Feb 2019 at 14:41, Thomas Koenig wrote: > > Hi, > > the attached patch fixes a 7/8/9 regression where a conversion warning > was emitted for DIM. The problem was that the no-warn flag had not been > passed down to the arithmetic conversion routines, which is solved here > by adding and using a flag in gfc_expr. > > Regression-tested. OK for affected branches? > > Regards > > Thomas > > 2019-02-02 Thomas Koenig > > PR fortran/88298 > * arith.c (gfc_int2int): Do not warn if src->do_not_warn is set. > * gfortran.h (gfc_expr): Add flag do_not_warn. > * intrinsic.c (gfc_convert_type_warn): Set expr->do_not_warn if > no warning is desired. > > 2019-02-02 Thomas Koenig > > PR fortran/88298 > * gfortran.dg/warn_conversion_10.f90: New test. -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
Re: Late-breaking jit features (was Re: [PATCH][gcc] libgccjit: introduce gcc_jit_context_add_driver_option)
On Sat, Feb 02, 2019 at 10:18:43AM -0500, David Malcolm wrote: > > > Alternatively, should these patches go into a branch of queued jit > > > changes for gcc 10? > > > > Is there anything like an ABI involved? If so we should avoid > > breaking it all the time. Otherwise JIT is not release critical and > > thus if you break it in the wrong moment it's your own fault. > > The two patches each add a new API entrypoint, but libgccjit uses > symbol-versioning to extend the ABI, without bumping the SONAME: > https://gcc.gnu.org/onlinedocs/jit/topics/compatibility.html > So it's not an ABI break as such. I'd say it depends on how quickly the copyright paperwork can be done, the patch can't be added until that is resolved. While gccjit is not release critical, it would be nice not to break it late, so say if it can be committed by end of February/mid March, I guess it is fine, given the assumption we'd like to release mid April to end of April, if it can't be done by then, might be better to postpone to GCC 10. Jakub
Re: Late-breaking jit features (was Re: [PATCH][gcc] libgccjit: introduce gcc_jit_context_add_driver_option)
On Sat, 2019-02-02 at 08:26 +0100, Richard Biener wrote: > On February 1, 2019 10:11:12 PM GMT+01:00, David Malcolm dhat.com> wrote: > > On Mon, 2019-01-21 at 08:40 +, Andrea Corallo wrote: > > > Hi all, > > > Second version of the patch addressing David's comment about all- > > > non- > > > failing-tests.h > > > > > > Adds gcc_jit_context_add_driver_option to the libgccjit ABI and a > > > testcase for it. > > > > > > Using this interface is now possible to pass options affecting > > > assembler and linker. > > > > > > Does not introduce regressions running make check-jit > > > > Thanks; the patch looks good. > > > > [CCing the release managers] > > > > Given that gcc development is now in stage 4, we really shouldn't > > be > > adding new features, but I'm wondering if an exception can be made > > for > > libgccjit? (this patch purely touches the jit subdirectories). > > > > There's one other late-breaking change, here: > > [PATCH][jit] Add thread-local globals to the libgccjit frontend > >https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00227.html > > which is nearly ready, but is awaiting copyright assignment > > paperwork. > > > > Alternatively, should these patches go into a branch of queued jit > > changes for gcc 10? > > Is there anything like an ABI involved? If so we should avoid > breaking it all the time. Otherwise JIT is not release critical and > thus if you break it in the wrong moment it's your own fault. The two patches each add a new API entrypoint, but libgccjit uses symbol-versioning to extend the ABI, without bumping the SONAME: https://gcc.gnu.org/onlinedocs/jit/topics/compatibility.html So it's not an ABI break as such. Dave > Richard. > > > Thanks > > Dave > > > > > > > Bests > > > > > > Andrea > > > > > > > > > gcc/jit/ChangeLog > > > 2019-01-16 Andrea Corallo andrea.cora...@arm.com > > > > > > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_11): New ABI tag. > > > * docs/topics/contexts.rst (Additional driver options): New > > > section. > > > * jit-playback.c (invoke_driver): Add call to > > > append_driver_options. > > > * jit-recording.c: Within namespace gcc::jit... > > > (recording::context::~context): Free the optnames within > > > m_driver_options. > > > (recording::context::add_driver_option): New method. > > > (recording::context::append_driver_options): New method. > > > (recording::context::dump_reproducer_to_file): Add driver > > > options. > > > * jit-recording.h: Within namespace gcc::jit... > > > (recording::context::add_driver_option): New method. > > > (recording::context::append_driver_options): New method. > > > (recording::context::m_driver_options): New field. > > > * libgccjit++.h (gccjit::context::add_driver_option): New > > > method. > > > * libgccjit.c (gcc_jit_context_add_driver_option): New API > > > entrypoint. > > > * libgccjit.h (gcc_jit_context_add_driver_option): New API > > > entrypoint. > > > (LIBGCCJIT_HAVE_gcc_jit_context_add_driver_option): New > > > macro. > > > * libgccjit.map (LIBGCCJIT_ABI_11): New ABI tag. > > > > > > > > > > > > gcc/testsuite/ChangeLog > > > 2019-01-16 Andrea Corallo andrea.cora...@arm.com > > > > > > * jit.dg/add-driver-options-testlib.c: Add support file for > > > test-add-driver-options.c testcase. > > > * jit.dg/all-non-failing-tests.h: Add note about > > > test-add-driver-options.c > > > * jit.dg/jit.exp (jit-dg-test): Update to support > > > add-driver-options-testlib.c compilation. > > > * jit.dg/test-add-driver-options.c: New testcase. > > > > >
[patch, fortran] Fix PR 88298
Hi,
the attached patch fixes a 7/8/9 regression where a conversion warning
was emitted for DIM. The problem was that the no-warn flag had not been
passed down to the arithmetic conversion routines, which is solved here
by adding and using a flag in gfc_expr.
Regression-tested. OK for affected branches?
Regards
Thomas
2019-02-02 Thomas Koenig
PR fortran/88298
* arith.c (gfc_int2int): Do not warn if src->do_not_warn is set.
* gfortran.h (gfc_expr): Add flag do_not_warn.
* intrinsic.c (gfc_convert_type_warn): Set expr->do_not_warn if
no warning is desired.
2019-02-02 Thomas Koenig
PR fortran/88298
* gfortran.dg/warn_conversion_10.f90: New test.
Index: arith.c
===
--- arith.c (Revision 268432)
+++ arith.c (Arbeitskopie)
@@ -2061,7 +2061,7 @@ gfc_int2int (gfc_expr *src, int kind)
gfc_convert_mpz_to_signed (result->value.integer,
gfc_integer_kinds[k].bit_size);
- if (warn_conversion && kind < src->ts.kind)
+ if (warn_conversion && !src->do_not_warn && kind < src->ts.kind)
gfc_warning_now (OPT_Wconversion, "Conversion from %qs to %qs at %L",
gfc_typename (>ts), gfc_typename (>ts),
>where);
Index: gfortran.h
===
--- gfortran.h (Revision 268432)
+++ gfortran.h (Arbeitskopie)
@@ -2168,6 +2168,9 @@ typedef struct gfc_expr
unsigned int do_not_resolve_again : 1;
+ /* Set this if no warning should be given somewhere in a lower level. */
+
+ unsigned int do_not_warn : 1;
/* If an expression comes from a Hollerith constant or compile-time
evaluation of a transfer statement, it may have a prescribed target-
memory representation, and these cannot always be backformed from
Index: intrinsic.c
===
--- intrinsic.c (Revision 268432)
+++ intrinsic.c (Arbeitskopie)
@@ -5028,6 +5028,8 @@ gfc_convert_type_warn (gfc_expr *expr, gfc_typespe
if (ts->type == BT_UNKNOWN)
goto bad;
+ expr->do_not_warn = ! wflag;
+
/* NULL and zero size arrays get their type here, unless they already have a
typespec. */
if ((expr->expr_type == EXPR_NULL
! { dg-do compile }
! { dg-options "-fno-range-check -Wconversion" }
! PR 88298 - this used to warn unnecessarily. Original test case by
! Harald Anlauf.
subroutine bug (j, js)
integer:: j, js(3,2)
js(:,:) = cshift (js(:,:), shift=j, dim=1)
end subroutine bug
Re: [PATCH] Fix not properly nul-terminated string constants in JIT
Sorry, Dave,
what should I do with this patch?
Bernd.
On 11/9/18 5:52 PM, Bernd Edlinger wrote:
> Hi Dave,
>
> is the patch OK, or do you still have questions?
>
>
> Thanks
> Bernd.
>
> On 11/2/18 10:48 PM, Bernd Edlinger wrote:
>> On 11/2/18 9:40 PM, David Malcolm wrote:
>>> On Sun, 2018-08-05 at 16:59 +, Bernd Edlinger wrote:
Hi!
My other patch with adds assertions to varasm.c regarding correct
nul termination of sting literals did make these incorrect string
constants in JIT frontend fail.
The string constants are not nul terminated if their length exceeds
200 characters. The test cases do not use strings of that size where
that would make a difference. But using a fixed index type is
clearly
wrong.
This patch removes the fixed char[200] array type from
playback::context,
and uses build_string_literal instead of using build_string directly.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
>>>
>>> Sorry for the belated response.
>>>
>>> Was this tested with --enable-host-shared and --enable-languages=jit ?
>>> Note that "jit" is not included in --enable-languages=all.
>>>
>>
>> Yes, of course. The test suite contains a few string constants, just
>> all of them are shorter than 200 characters. But I think removing this
>> artificial limit enables the existing test cases to test that the
>> shorter string is in fact zero terminated.
>>
>>> The patch seems reasonable, but I'm a little confused over the meaning
>>> of "len" in build_string_literal and build_string: does it refer to the
>>> length or the size of the string?
>>>
>>
>> build_string_literal:
>> For languages that use zero-terminated strings, len is strlen(str)+1, and
>> str is a zero terminated single-byte character string.
>> For languages that don't use zero-terminated strings, len is the size of
>> the string and str is not zero terminated.
>>
>> build_string:
>> constructs a STRING_CST tree object, which is usable as is in some contexts,
>> like for asm constraints, but as a string literal it is incomplete, and
>> needs an index type. The index type defines the memory size which must
>> be larger than the string precision. Excess memory is implicitly cleared.
>>
>> This means currently all jit strings shorter than 200 characters
>> are filled with zero up to the limit of 200 chars as imposed by
>> m_char_array_type_node. Strings of exactly 200 chars are not zero
>> terminated,
>> and larger strings should result in an assertion (excess precision was
>> previously
>> allowed, but no zero termination was appended, when that is not part of
>> the original string constant).
>>
>> Previously it was allowed to have memory size less than the string len, which
>> had complicated the STRING_CST semantics in the middle-end, but with the
>> string_cst semantic rework I did for gcc-9 this is no longer allowed and
>> results in (checking) assertions in varasm.c.
>>
@@ -617,16 +616,9 @@ playback::rvalue *
playback::context::
new_string_literal (const char *value)
{
- tree t_str = build_string (strlen (value), value);
- gcc_assert (m_char_array_type_node);
- TREE_TYPE (t_str) = m_char_array_type_node;
-
- /* Convert to (const char*), loosely based on
- c/c-typeck.c: array_to_pointer_conversion,
- by taking address of start of string. */
- tree t_addr = build1 (ADDR_EXPR, m_const_char_ptr, t_str);
+ tree t_str = build_string_literal (strlen (value) + 1, value);
- return new rvalue (this, t_addr);
+ return new rvalue (this, t_str);
}
>>>
>>> In the above, the call to build_string with strlen is replaced with
>>> build_string_literal with strlen + 1.
>>>
>>>
>>> build_string's comment says:
>>>
>>> "Note that for a C string literal, LEN should include the trailing
>>> NUL."
>>>
>>> but has:
>>>
>>> length = len + offsetof (struct tree_string, str) + 1;
>>>
>>> and:
>>>
>>> TREE_STRING_LENGTH (s) = len;
>>> memcpy (s->string.str, str, len);
>>> s->string.str[len] = '\0';
>>>
>>> suggesting that the "len" parameter is in fact the length *without* the
>>> trailing NUL, and that a trailing NUL is added by build_string.
>>>
>>
>> Yes, string constants in tree objects have another zero termiation,
>> but varasm.c does something different, there the index range takes
>> precedence.
>> The index range is built in build_string_literal as follows:
>>
>> elem = build_type_variant (char_type_node, 1, 0);
>> index = build_index_type (size_int (len - 1));
>> type = build_array_type (elem, index);
>>
>> therefore the string constant hast the type char[0..len-1]
>> thus only len bytes are significant for code generation, the extra
>> nul is just for "convenience".
>>
>>> However build_string_literal has:
>>>
>>> t = build_string (len, str);
>>> elem = build_type_variant (char_type_node, 1, 0);
>>>
[PATCH] Fix up gcc.target/i386/call-1.c testcase (PR rtl-optimization/11304)
On Sat, Feb 02, 2019 at 11:22:55AM +0100, Jakub Jelinek wrote:
> The only "regression" was gcc.target/i386/call-1.c with
> -fstack-protector-strong, but that is because the test is invalid:
> void set_eax(int val)
> {
> __asm__ __volatile__ ("mov %0, %%eax" : : "m" (val));
> }
> - missing "eax" clobber or "=a" (dummy) output and when set_eax is inlined
> that can break optimizations badly. Of course, in addition to fixing that,
> I'd expect if the tests wants to test what it originally wanted to test, it
> needs to disable inlining or perhaps all IPA opts, not sure if just for
> set_eax or also for foo/bar. Perhaps we can keep the testcase as is with
> the "eax" clobber and add another one with __attribute__((noipa)) on
> set_eax/foo/bar.
Regardless of the PR87485 decision, I think we should fix this testcase.
So here it is in patch form, regtested on x86_64-linux
(\{-m32,-m32/-fstack-protector-strong,-m64,-m64/-fstack-protector-strong\}),
ok for trunk?
2019-02-02 Jakub Jelinek
PR rtl-optimization/11304
* gcc.target/i386/call-1.c (set_eax): Add "eax" clobber.
* gcc.target/i386/call-2.c: New test.
--- gcc/testsuite/gcc.target/i386/call-1.c.jj 2008-09-05 12:54:23.0
+0200
+++ gcc/testsuite/gcc.target/i386/call-1.c 2019-02-02 11:23:25.566902736
+0100
@@ -11,7 +11,7 @@ volatile int r;
void set_eax(int val)
{
- __asm__ __volatile__ ("mov %0, %%eax" : : "m" (val));
+ __asm__ __volatile__ ("mov %0, %%eax" : : "m" (val) : "eax");
}
void foo(int val)
--- gcc/testsuite/gcc.target/i386/call-2.c.jj 2019-02-02 11:23:31.922797178
+0100
+++ gcc/testsuite/gcc.target/i386/call-2.c 2019-02-02 11:23:47.757534186
+0100
@@ -0,0 +1,12 @@
+/* PR optimization/11304 */
+/* Originator: */
+/* { dg-do run } */
+/* { dg-options "-O -fomit-frame-pointer" } */
+
+/* Verify that %eax is always restored after a call. */
+
+__attribute__((noipa)) void set_eax(int val);
+__attribute__((noipa)) void foo(int val);
+__attribute__((noipa)) int bar(int x);
+
+#include "call-1.c"
Jakub
Re: [PATCH, libphobos] Detect if qsort_r is available (PR d/88127)
On Sat, Feb 02, 2019 at 11:01:10AM +0100, Johannes Pfau wrote: > Adds a configure test for qsort_r and use the fallback code path if > it's not available. Fixes d/88127. rt/qsort.d changes have been > pushed upstream and reviewed there: > https://github.com/dlang/druntime/pull/2480 > Bootstrapped & ran D test suite on x86_64_linux with a recent glibc, > checked that Have_Qsort_R is set correctly in config.d. > > libphobos/ChangeLog: > > 2019-02-02 Johannes Pfau > > * m4/druntime/libraries.m4: Add check for qsort_r as > DRUNTIME_LIBRARIES_CLIB. Just a small nit, this line is too long, should be wrapped. Will defer to Iain for actual review. Jakub
Re: [PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
On Fri, Feb 01, 2019 at 11:52:04PM +0100, Eric Botcazou wrote:
> > So, can we e.g. keep emitting the epilogue where it is now for
> > naked_return_label != NULL_RTX and move it otherwise?
> > For __builtin_return the setter and use of the hard register won't be
> > adjacent in any case.
>
> See my comment in the audit trail of the PR; I'd suspend it and go to bed. ;-)
While the set of -fno- and -f options in some PRs are unlikely to be used by
people in the wild, often those PRs uncover latent bugs that could cause
serious wrong-code or ICEs, of course not always. So IMHO we shouldn't
ignore those PRs, especially if they are regressions.
In the meantime, I've bootstrapped/regtested successfully following version
of the patch that should fix the builtin return case.
In addition to normal {x86_64,i686}-linux bootstrap/regtest I've done a
distro build where we 1) build the compiler itself with
-fstack-protector-strong 2) run testsuite with
--tool-test=\{,-fstack-protector-strong\}, so far on
{x86_64,i686,powerpc64le}-linux, other targets still pending.
The only "regression" was gcc.target/i386/call-1.c with
-fstack-protector-strong, but that is because the test is invalid:
void set_eax(int val)
{
__asm__ __volatile__ ("mov %0, %%eax" : : "m" (val));
}
- missing "eax" clobber or "=a" (dummy) output and when set_eax is inlined
that can break optimizations badly. Of course, in addition to fixing that,
I'd expect if the tests wants to test what it originally wanted to test, it
needs to disable inlining or perhaps all IPA opts, not sure if just for
set_eax or also for foo/bar. Perhaps we can keep the testcase as is with
the "eax" clobber and add another one with __attribute__((noipa)) on
set_eax/foo/bar.
2019-02-01 Jakub Jelinek
PR rtl-optimization/87485
* function.c (expand_function_end): Move stack_protect_epilogue
before loading of return value into hard register(s).
* gcc.dg/pr87485.c: New test.
--- gcc/function.c.jj 2019-01-29 16:47:02.0 +0100
+++ gcc/function.c 2019-02-01 16:23:07.471877843 +0100
@@ -5330,6 +5330,12 @@ expand_function_end (void)
communicate between __builtin_eh_return and the epilogue. */
expand_eh_return ();
+ /* If stack protection is enabled for this function, check the guard. */
+ if (crtl->stack_protect_guard
+ && targetm.stack_protect_runtime_enabled_p ()
+ && naked_return_label == NULL_RTX)
+stack_protect_epilogue ();
+
/* If scalar return value was computed in a pseudo-reg, or was a named
return value that got dumped to the stack, copy that to the hard
return register. */
@@ -5476,7 +5482,9 @@ expand_function_end (void)
emit_insn (gen_blockage ());
/* If stack protection is enabled for this function, check the guard. */
- if (crtl->stack_protect_guard && targetm.stack_protect_runtime_enabled_p ())
+ if (crtl->stack_protect_guard
+ && targetm.stack_protect_runtime_enabled_p ()
+ && naked_return_label)
stack_protect_epilogue ();
/* If we had calls to alloca, and this machine needs
--- gcc/testsuite/gcc.dg/pr87485.c.jj 2019-02-01 16:30:51.101211900 +0100
+++ gcc/testsuite/gcc.dg/pr87485.c 2019-02-01 16:31:48.660260183 +0100
@@ -0,0 +1,29 @@
+/* PR rtl-optimization/87485 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -fschedule-insns -fno-guess-branch-probability
-fno-isolate-erroneous-paths-dereference -fno-omit-frame-pointer
-fno-split-wide-types -fno-tree-ccp -fno-tree-sra" } */
+/* { dg-additional-options "-fstack-protector-strong" { target
fstack_protector } } */
+
+int *a;
+
+int
+foo (__int128 x, int y, int z)
+{
+ __int128 b;
+ *a = ((!!y ? y : x) * y | x) * 2;
+ if (z == 0)
+{
+ unsigned int c = 1;
+ __int128 *d =
+ for (*a = 0; *a < 1; *a += y)
+ ;
+ *a += b < (c / 0); /* { dg-warning "division by zero" } */
+ goto l;
+ m:
+ while (b < 1)
+ ;
+ ++*a;
+}
+ goto m;
+ l:
+ return 0;
+}
Jakub
[PATCH, libphobos] Detect if qsort_r is available (PR d/88127)
Adds a configure test for qsort_r and use the fallback code path if
it's not available. Fixes d/88127. rt/qsort.d changes have been
pushed upstream and reviewed there: https://github.com/dlang/druntime/pull/2480
Bootstrapped & ran D test suite on x86_64_linux with a recent glibc,
checked that Have_Qsort_R is set correctly in config.d.
libphobos/ChangeLog:
2019-02-02 Johannes Pfau
* m4/druntime/libraries.m4: Add check for qsort_r as
DRUNTIME_LIBRARIES_CLIB.
* configure.ac: Use qsort_r check.
* libdruntime/gcc/config.d.in: Add Have_Qsort_R to store check result.
* libdruntime/rt/qsort.d: Check Have_Qsort_R before using qsort_r.
* Makefile.in: Regenerate.
* aclocal.m4: Regenerate.
* configure: Regenerate.
* libdruntime/Makefile.in: Regenerate.
* src/Makefile.in: Regenerate.
* testsuite/Makefile.in: Regenerate.
---
libphobos/Makefile.in | 7 +++--
libphobos/aclocal.m4 | 40 +--
libphobos/configure | 26 +++--
libphobos/configure.ac| 1 +
libphobos/libdruntime/Makefile.in | 7 +++--
libphobos/libdruntime/gcc/config.d.in | 3 ++
libphobos/libdruntime/rt/qsort.d | 18
libphobos/m4/druntime/libraries.m4| 12
libphobos/src/Makefile.in | 5 ++--
libphobos/testsuite/Makefile.in | 5 ++--
10 files changed, 92 insertions(+), 32 deletions(-)
diff --git a/libphobos/configure.ac b/libphobos/configure.ac
index 919bc194af4..a0cd9bc9546 100644
--- a/libphobos/configure.ac
+++ b/libphobos/configure.ac
@@ -126,6 +126,7 @@ DRUNTIME_OS_SOURCES
DRUNTIME_OS_THREAD_MODEL
DRUNTIME_OS_ARM_EABI_UNWINDER
DRUNTIME_OS_MINFO_BRACKETING
+DRUNTIME_LIBRARIES_CLIB
WITH_LOCAL_DRUNTIME([
AC_LANG_PUSH([D])
diff --git a/libphobos/libdruntime/gcc/config.d.in
b/libphobos/libdruntime/gcc/config.d.in
index 3a1d493f3c4..803adb90c4a 100644
--- a/libphobos/libdruntime/gcc/config.d.in
+++ b/libphobos/libdruntime/gcc/config.d.in
@@ -46,3 +46,6 @@ enum GNU_Have_64Bit_Atomics = @DCFG_HAVE_64BIT_ATOMICS@;
// Do we have libatomic available
enum GNU_Have_LibAtomic = @DCFG_HAVE_LIBATOMIC@;
+
+// Do we have qsort_r function
+enum Have_Qsort_R = @DCFG_HAVE_QSORT_R@;
diff --git a/libphobos/libdruntime/rt/qsort.d b/libphobos/libdruntime/rt/qsort.d
index 6c3e71c35c7..af0c1eb704a 100644
--- a/libphobos/libdruntime/rt/qsort.d
+++ b/libphobos/libdruntime/rt/qsort.d
@@ -27,7 +27,25 @@ else version (TVOS)
else version (WatchOS)
version = Darwin;
+// qsort_r was added in glibc in 2.8.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88127
version (CRuntime_Glibc)
+{
+version (GNU)
+{
+import gcc.config : Have_Qsort_R;
+enum Glibc_Qsort_R = Have_Qsort_R;
+}
+else
+{
+enum Glibc_Qsort_R = true;
+}
+}
+else
+{
+enum Glibc_Qsort_R = false;
+}
+
+static if (Glibc_Qsort_R)
{
alias extern (C) int function(scope const void *, scope const void *,
scope void *) Cmp;
extern (C) void qsort_r(scope void *base, size_t nmemb, size_t size, Cmp
cmp, scope void *arg);
diff --git a/libphobos/m4/druntime/libraries.m4
b/libphobos/m4/druntime/libraries.m4
index 17f93468b87..35a791d137a 100644
--- a/libphobos/m4/druntime/libraries.m4
+++ b/libphobos/m4/druntime/libraries.m4
@@ -161,3 +161,15 @@ AC_DEFUN([DRUNTIME_LIBRARIES_BACKTRACE],
AC_SUBST(BACKTRACE_SUPPORTS_THREADS)
AC_LANG_POP([C])
])
+
+# DRUNTIME_LIBRARIES_CLIB
+# ---
+# Perform various feature checks on the C library.
+AC_DEFUN([DRUNTIME_LIBRARIES_CLIB],
+[
+ AC_LANG_PUSH([C])
+ DCFG_HAVE_QSORT_R=false
+ AC_CHECK_FUNC(qsort_r, [DCFG_HAVE_QSORT_R=true])
+ AC_SUBST(DCFG_HAVE_QSORT_R)
+ AC_LANG_POP([C])
+])
Re: [Patch, fortran] PR88393 - [7/8/9 Regression] [OOP] Segfault with type-bound assignment
Unfortunately, it doesn't. I have taken it though since it should pretty low hanging fruit. Cheers Paul On Fri, 1 Feb 2019 at 19:31, Steve Kargl wrote: > > On Fri, Feb 01, 2019 at 06:15:21PM +, Paul Richard Thomas wrote: > > I will commit this patch as 'obvious' tomorrow. > > > > Cheers > > > > Paul > > > > 2019-02-01 Paul Thomas > > > > PR fortran/88393 > > * trans-expr.c (gfc_conv_procedure_call): For derived entities, > > passed in parentheses to class formals, invert the order of > > copying allocatable components to taking taking the _data of > > the class expression. > > > > 2019-02-01 Paul Thomas > > > > PR fortran/88393 > > * gfortran.dg/alloc_comp_assign_16.f03 : New test. > > Paul, > > Does this patch also fix PR57710? > > -- > Steve -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
Re: [Patch, fortran] PR88393 - [7/8/9 Regression] [OOP] Segfault with type-bound assignment
Hi Steve,
> taking taking
>
OK OK
> > Index: gcc/fortran/trans-expr.c
> > ===
> > *** gcc/fortran/trans-expr.c (revision 268231)
> > --- gcc/fortran/trans-expr.c (working copy)
> > *** gfc_conv_procedure_call (gfc_se * se, gf
> > *** 6042,6047
> > --- 6042,6057
> > break;
> > }
> >
> > + if (e->ts.type == BT_DERIVED && fsym && fsym->ts.type == BT_CLASS)
> > + {
> > + /* The derived type is passed to gfc_deallocate_alloc_comp.
> > + Therefore, class actuals can handled correctly but derived
>
> s/can handled/can be handled/
Thanks - in the original of course but I should have spotted it.
Thanks for the review.
Paul
