public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors
@ 2021-01-02 11:11 slyfox at gcc dot gnu.org
2021-01-02 22:22 ` [Bug c++/98499] " slyfox at gcc dot gnu.org
` (13 more replies)
0 siblings, 14 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-02 11:11 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
Bug ID: 98499
Summary: [11 Regression] Possibly bad std::string
initialization in constructors
Product: gcc
Version: 11.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: c++
Assignee: unassigned at gcc dot gnu.org
Reporter: slyfox at gcc dot gnu.org
Target Milestone: ---
Initially bug is observed on a usage crash of libsass-3.6.4. Code snippet
around the crash:
https://github.com/sass/libsass/blob/3.6.4/src/context.cpp#L621
I think I extracted a small example that illustrates the problem:
```c++
// cat main.cc
#include <string>
__attribute__((noinline))
static std::string dir_name() { return "c"; }
__attribute__((noinline))
static std::string make_canonical_path (std::string path) { return path; }
class Importer {
public:
std::string imp_path;
std::string ctx_path;
std::string base_path;
public:
__attribute__((noinline)) Importer(std::string imp_path, std::string
ctx_path)
: imp_path(make_canonical_path(imp_path))
, ctx_path(make_canonical_path(ctx_path))
, base_path(dir_name())
{}
};
struct Include {
Include(const Importer& imp){}
};
int main() {
const Include & inc = {{"a", "b"}};
}
```
g++-11 generates crashing binaries, g++-10 does not:
```
$ g++-11.0.0 -O2 -std=c++11 main.cc -o a-11; ./a-11; echo $?
free(): invalid pointer
Aborted (core dumped)
134
$ g++-10.2.0 -O2 -std=c++11 main.cc -o a-10; ./a-10; echo $?
0
```
I was not able to easily get rid of std::string as it uses something from
libstdc++.so.
Thus I'm not sure where the bug is. My suspictions are:
1. invalid c++
2. std::string implementation bug
3. g++'s code generation problem around lifetimes of temporary values
I suspect `[3.]`.
```
$ g++-11.0.0 -v
Using built-in specs.
COLLECT_GCC=/usr/bin/g++-11.0.0
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with:
/var/tmp/portage/sys-devel/gcc-11.0.0_pre9999/work/gcc-11.0.0_pre9999/configure
--host=x86_64-pc-linux-gnu --build=x86_64-pc-linux-gnu --prefix=/usr
--bindir=/usr/x86_64-pc-linux-gnu/gcc-bin/11.0.0
--includedir=/usr/lib/gcc/x86_64-pc-linux-gnu/11.0.0/include
--datadir=/usr/share/gcc-data/x86_64-pc-linux-gnu/11.0.0
--mandir=/usr/share/gcc-data/x86_64-pc-linux-gnu/11.0.0/man
--infodir=/usr/share/gcc-data/x86_64-pc-linux-gnu/11.0.0/info
--with-gxx-include-dir=/usr/lib/gcc/x86_64-pc-linux-gnu/11.0.0/include/g++-v11
--with-python-dir=/share/gcc-data/x86_64-pc-linux-gnu/11.0.0/python
--enable-languages=c,c++,go,jit,fortran --enable-obsolete --enable-secureplt
--disable-werror --with-system-zlib --enable-nls --without-included-gettext
--enable-checking=release --with-bugurl=https://bugs.gentoo.org/
--with-pkgversion='Gentoo 11.0.0_pre9999 p5, commit
12ae2bc70846a2be8255eaa41322cd1a5a7b7350' --disable-esp --enable-libstdcxx-time
--enable-host-shared --enable-shared --enable-threads=posix
--enable-__cxa_atexit --enable-clocale=gnu --enable-multilib
--with-multilib-list=m32,m64 --disable-fixed-point --enable-targets=all
--enable-libgomp --disable-libssp --disable-libada --disable-systemtap
--enable-valgrind-annotations --enable-vtable-verify --with-zstd --enable-lto
--with-isl --disable-isl-version-check --enable-default-pie
--enable-default-ssp
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.0.0 20201228 (experimental) (Gentoo 11.0.0_pre9999 p5, commit
12ae2bc70846a2be8255eaa41322cd1a5a7b7350)
```
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug c++/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
@ 2021-01-02 22:22 ` slyfox at gcc dot gnu.org
2021-01-03 11:44 ` slyfox at gcc dot gnu.org
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-02 22:22 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #1 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
Managed to get rid of external <string> dependency:
```
struct string {
char * _M_buf;
// local store
char _M_local_buf[16];
string(const string &__str) : _M_buf(_M_local_buf) {}
string(const char *__s) : _M_buf(_M_local_buf) {}
~string() {
if (_M_buf != _M_local_buf)
__builtin_trap();
}
// not defined
string();
};
// main.cc
__attribute__((noinline)) static string dir_name() { return "c"; }
__attribute__((noinline)) static string make_canonical_path(string path) {
return path;
}
class Importer {
public:
string imp_path;
string ctx_path;
string base_path;
public:
__attribute__((noinline)) Importer(string imp_path, string ctx_path)
: imp_path(make_canonical_path(imp_path)),
ctx_path(make_canonical_path(ctx_path)), base_path(dir_name()) {}
};
struct Include {
Include(const Importer &imp) {}
};
int main() { const Include &inc = {{"a", "b"}}; }
```
Note: all our string() constructors shoudl have `_M_buf == _M_local_buf`
invariant. But gcc-11 generates always-crashing program:
```
; g++-11.0.0 -O2 -std=c++11 -S main.cc
main:
.cfi_startproc
subq $152, %rsp
.cfi_def_cfa_offset 160
movq %fs:40, %rax
movq %rax, 136(%rsp)
xorl %eax, %eax
leaq 8(%rsp), %rax
movq %rsp, %rsi
leaq 32(%rsp), %rdx
movq %rax, (%rsp)
leaq 64(%rsp), %rdi
leaq 40(%rsp), %rax
movq %rax, 32(%rsp)
call _ZN8ImporterC1E6stringS0_
ud2
```
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug c++/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
2021-01-02 22:22 ` [Bug c++/98499] " slyfox at gcc dot gnu.org
@ 2021-01-03 11:44 ` slyfox at gcc dot gnu.org
2021-01-03 20:56 ` slyfox at gcc dot gnu.org
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-03 11:44 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
Sergei Trofimovich <slyfox at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jh at suse dot cz
--- Comment #2 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
Bisected down to:
```
commit 520d5ad337eaa15860a5a964daf7ca46cf31c029
Author: Jan Hubicka <jh@suse.cz>
Date: Sat Nov 14 13:52:36 2020 +0100
Detect EAF flags in ipa-modref
A minimal patch for the EAF flags discovery. It works only in local
ipa-modref
and gives up on cyclic SSA graphs. It improves pt_solution_includes
disambiguations twice.
```
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug c++/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
2021-01-02 22:22 ` [Bug c++/98499] " slyfox at gcc dot gnu.org
2021-01-03 11:44 ` slyfox at gcc dot gnu.org
@ 2021-01-03 20:56 ` slyfox at gcc dot gnu.org
2021-01-03 21:50 ` slyfox at gcc dot gnu.org
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-03 20:56 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #3 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
`--param=modref-max-depth=0` makes the bug disappear.
Looking at `-fdump-tree-all` the harmful optimization happens at `107.fre3`
where:
```
if (&MEM[(struct string *)&D.2237 + 48B]._M_local_buf != _17)
goto <bb 3>; [0.00%]
else
goto <bb 4>; [100.00%]
<bb 3> [count: 0]:
__builtin_trap ();
```
is converted into an unconditional `__builtin_trap ();`. `-fdump-tree-all-all`
says it's a constant fold:
```
Value numbering stmt = if (&MEM[(struct string *)&D.2237 + 48B]._M_local_buf !=
_17)
Applying pattern match.pd:4920, gimple-match.c:3373
marking known outgoing edge 2 -> 3 executable
Block 1: BB4 found not executable
...
```
It looks like it's constant-folded into always-true instead of expected
always-false. `match.pd:4920` is a `pta`:
```
/* Simplify pointer equality compares using PTA. */
(for neeq (ne eq)
(simplify
(neeq @0 @1)
(if (POINTER_TYPE_P (TREE_TYPE (@0))
&& ptrs_compare_unequal (@0, @1))
{ constant_boolean_node (neeq != EQ_EXPR, type); })))
```
`036t.ealias` and `043t.modref1` says `--param=modref-max-depth=0` has the
effect on how many things escape from main(), but I don't understand the
output.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug c++/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (2 preceding siblings ...)
2021-01-03 20:56 ` slyfox at gcc dot gnu.org
@ 2021-01-03 21:50 ` slyfox at gcc dot gnu.org
2021-01-04 12:28 ` marxin at gcc dot gnu.org
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-03 21:50 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #4 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
Managed to shrink example even further. Now fails in `037t.fre1`:
```c++
struct string {
char * _M_buf;
// local store
char _M_local_buf[16];
__attribute__((noinline)) string() : _M_buf(_M_local_buf) {}
~string() {
if (_M_buf != _M_local_buf)
__builtin_trap();
}
string(const string &__str); // no copies
};
// main.cc
__attribute__((noinline)) static string dir_name() { return string(); }
class Importer {
string base_path;
public:
__attribute__((noinline)) Importer() : base_path (dir_name()) {}
};
int main() {
Importer imp;
}
```
```
$ g++-11.0.0 -O2 -std=c++11 main.cc -o a && ./a
Illegal instruction (core dumped) ./a
$ g++-11.0.0 --param=modref-max-depth=0 -O2 -std=c++11 main.cc -o a && ./a
<ok>
```
It feels like c++'s return-value-optimization somehow misses the PTA.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug c++/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (3 preceding siblings ...)
2021-01-03 21:50 ` slyfox at gcc dot gnu.org
@ 2021-01-04 12:28 ` marxin at gcc dot gnu.org
2021-01-05 11:09 ` rguenth at gcc dot gnu.org
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: marxin at gcc dot gnu.org @ 2021-01-04 12:28 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
Martin Liška <marxin at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |marxin at gcc dot gnu.org
Ever confirmed|0 |1
Target Milestone|--- |11.0
Priority|P3 |P1
Status|UNCONFIRMED |NEW
Last reconfirmed| |2021-01-04
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug c++/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (4 preceding siblings ...)
2021-01-04 12:28 ` marxin at gcc dot gnu.org
@ 2021-01-05 11:09 ` rguenth at gcc dot gnu.org
2021-01-06 23:11 ` [Bug tree-optimization/98499] " slyfox at gcc dot gnu.org
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rguenth at gcc dot gnu.org @ 2021-01-05 11:09 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Keywords| |wrong-code
CC| |rguenth at gcc dot gnu.org
--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Possibly in discovering pure/constness. See uses of
gimple_call_return_slot_opt_p in tree-ssa-structalias.c
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (5 preceding siblings ...)
2021-01-05 11:09 ` rguenth at gcc dot gnu.org
@ 2021-01-06 23:11 ` slyfox at gcc dot gnu.org
2021-01-07 8:12 ` rguenth at gcc dot gnu.org
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-06 23:11 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #6 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> Possibly in discovering pure/constness. See uses of
> gimple_call_return_slot_opt_p in tree-ssa-structalias.c
Aha, that's useful!
Trying to understand the problem better myself: `-fdump-tree-all` has seemingly
relevant `036t.ealias` that precedes breaking `037t.fre1`.
I assume `036t.ealias` analyses individual functions locally and does not take
into account other details, thus main() analysis should be enough:
```
...
Points-to sets
ANYTHING = { ANYTHING }
ESCAPED = { ESCAPED NONLOCAL }
NONLOCAL = { ESCAPED NONLOCAL }
STOREDANYTHING = { }
INTEGER = { ANYTHING }
_ZN8ImporterC1Ev = { }
imp.0+64 = { ESCAPED NONLOCAL } same as _6
imp.64+8 = { ESCAPED NONLOCAL }
__builtin_trap = { }
main = { }
CALLUSED(9) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 } same as callarg(11)
CALLCLOBBERED(10) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 } same as callarg(11)
callarg(11) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 }
_6 = { ESCAPED NONLOCAL }
Alias information for int main()
Aliased symbols
imp, UID D.2146, struct Importer, is addressable
Call clobber information
ESCAPED, points-to non-local, points-to vars: { }
Flow-insensitive points-to information
_6, points-to non-local, points-to escaped, points-to NULL, points-to vars: { }
int main ()
{
struct Importer imp;
char * _6;
<bb 2> :
Importer::Importer (&imp);
_6 = MEM[(struct string *)&imp]._M_buf;
if (&MEM[(struct string *)&imp]._M_local_buf != _6)
goto <bb 3>; [0.00%]
else
goto <bb 4>; [100.00%]
<bb 3> [count: 0]:
__builtin_trap ();
<bb 4> :
imp ={v} {CLOBBER};
imp ={v} {CLOBBER};
return 0;
}
```
I think this looks correct. As I understand we care about a few things in the
analysis here:
1. imp.0+64 and _6 both point to the same flow-insensitive classes (both are
ESCAPED NONLOCAL)
2. imp.0+64 and _6 both point to the same field in flow-sensitive analysis
(both do according to `imp.0+64 = { ESCAPED NONLOCAL } same as _6`.
I don't see problems here.
Mechanically looking at incorrect gcc's decision for `imp.0+64 != _6`:
ptrs_compare_unequal(
ptr1 = &MEM[(struct string *)&imp]._M_local_buf,
ptr2 = _6
)
returns `TRUE` because
pt_solution_includes(
info = ptr2->pt,
obj1 = imp
)
returns `FALSE`. That seems to be a bug.
Do arguments to `pt_solution_includes` look correct so far? Does it try to
answer "could _6 point at any field of 'imp' type"?
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (6 preceding siblings ...)
2021-01-06 23:11 ` [Bug tree-optimization/98499] " slyfox at gcc dot gnu.org
@ 2021-01-07 8:12 ` rguenth at gcc dot gnu.org
2021-01-10 18:39 ` slyfox at gcc dot gnu.org
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rguenth at gcc dot gnu.org @ 2021-01-07 8:12 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Sergei Trofimovich from comment #6)
> (In reply to Richard Biener from comment #5)
> > Possibly in discovering pure/constness. See uses of
> > gimple_call_return_slot_opt_p in tree-ssa-structalias.c
>
> Aha, that's useful!
>
> Trying to understand the problem better myself: `-fdump-tree-all` has
> seemingly relevant `036t.ealias` that precedes breaking `037t.fre1`.
>
> I assume `036t.ealias` analyses individual functions locally and does not
> take into account other details, thus main() analysis should be enough:
Well - it does take into account fnspecs derived by modref analysis for
Importer::Importer - specifically ...
> ```
> ...
> Points-to sets
>
> ANYTHING = { ANYTHING }
> ESCAPED = { ESCAPED NONLOCAL }
> NONLOCAL = { ESCAPED NONLOCAL }
> STOREDANYTHING = { }
> INTEGER = { ANYTHING }
> _ZN8ImporterC1Ev = { }
> imp.0+64 = { ESCAPED NONLOCAL } same as _6
> imp.64+8 = { ESCAPED NONLOCAL }
> __builtin_trap = { }
> main = { }
> CALLUSED(9) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 } same as callarg(11)
> CALLCLOBBERED(10) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 } same as
> callarg(11)
> callarg(11) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 }
the above shows we do not consider 'imp' to escape, and thus
> _6 = { ESCAPED NONLOCAL }
_6 does not point to 'imp'.
Relevant parts of handle_rhs_call are probably
/* As we compute ESCAPED context-insensitive we do not gain
any precision with just EAF_NOCLOBBER but not EAF_NOESCAPE
set. The argument would still get clobbered through the
escape solution. */
if ((flags & EAF_NOCLOBBER)
&& (flags & (EAF_NOESCAPE | EAF_NODIRECTESCAPE)))
{
...
specifically lines
if (!(flags & (EAF_NOESCAPE | EAF_DIRECT)))
make_indirect_escape_constraint (tem);
probably do not trigger because of the invalid modref analysis. I suggest
to look at the early modref pass dump (it's after FRE but still applies
to callers)
>
> Alias information for int main()
>
> Aliased symbols
>
> imp, UID D.2146, struct Importer, is addressable
>
> Call clobber information
>
> ESCAPED, points-to non-local, points-to vars: { }
>
> Flow-insensitive points-to information
>
> _6, points-to non-local, points-to escaped, points-to NULL, points-to vars:
> { }
>
> int main ()
> {
> struct Importer imp;
> char * _6;
>
> <bb 2> :
> Importer::Importer (&imp);
> _6 = MEM[(struct string *)&imp]._M_buf;
> if (&MEM[(struct string *)&imp]._M_local_buf != _6)
> goto <bb 3>; [0.00%]
> else
> goto <bb 4>; [100.00%]
>
> <bb 3> [count: 0]:
> __builtin_trap ();
>
> <bb 4> :
> imp ={v} {CLOBBER};
> imp ={v} {CLOBBER};
> return 0;
> }
> ```
>
> I think this looks correct. As I understand we care about a few things in
> the analysis here:
> 1. imp.0+64 and _6 both point to the same flow-insensitive classes (both are
> ESCAPED NONLOCAL)
> 2. imp.0+64 and _6 both point to the same field in flow-sensitive analysis
> (both do according to `imp.0+64 = { ESCAPED NONLOCAL } same as _6`.
>
> I don't see problems here.
>
> Mechanically looking at incorrect gcc's decision for `imp.0+64 != _6`:
>
> ptrs_compare_unequal(
> ptr1 = &MEM[(struct string *)&imp]._M_local_buf,
> ptr2 = _6
> )
>
> returns `TRUE` because
>
> pt_solution_includes(
> info = ptr2->pt,
> obj1 = imp
> )
>
> returns `FALSE`. That seems to be a bug.
>
> Do arguments to `pt_solution_includes` look correct so far? Does it try to
> answer "could _6 point at any field of 'imp' type"?
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (7 preceding siblings ...)
2021-01-07 8:12 ` rguenth at gcc dot gnu.org
@ 2021-01-10 18:39 ` slyfox at gcc dot gnu.org
2021-01-28 10:55 ` hubicka at gcc dot gnu.org
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-10 18:39 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #8 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #7)
> (In reply to Sergei Trofimovich from comment #6)
> > (In reply to Richard Biener from comment #5)
> > > Possibly in discovering pure/constness. See uses of
> > > gimple_call_return_slot_opt_p in tree-ssa-structalias.c
> >
> > Aha, that's useful!
> >
> > Trying to understand the problem better myself: `-fdump-tree-all` has
> > seemingly relevant `036t.ealias` that precedes breaking `037t.fre1`.
> >
> > I assume `036t.ealias` analyses individual functions locally and does not
> > take into account other details, thus main() analysis should be enough:
>
> Well - it does take into account fnspecs derived by modref analysis for
> Importer::Importer - specifically ...
Oh, thank you! Only after many printf() attempts it sunk in that `036t.ealias`
is using data from seemingly later `043t.modref1` pass. It is so confusing!
> > ```
> > ...
> > Points-to sets
> >
> > ANYTHING = { ANYTHING }
> > ESCAPED = { ESCAPED NONLOCAL }
> > NONLOCAL = { ESCAPED NONLOCAL }
> > STOREDANYTHING = { }
> > INTEGER = { ANYTHING }
> > _ZN8ImporterC1Ev = { }
> > imp.0+64 = { ESCAPED NONLOCAL } same as _6
> > imp.64+8 = { ESCAPED NONLOCAL }
> > __builtin_trap = { }
> > main = { }
> > CALLUSED(9) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 } same as callarg(11)
> > CALLCLOBBERED(10) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 } same as
> > callarg(11)
> > callarg(11) = { ESCAPED NONLOCAL imp.0+64 imp.64+8 }
>
> the above shows we do not consider 'imp' to escape, and thus
>
> > _6 = { ESCAPED NONLOCAL }
>
> _6 does not point to 'imp'.
>
> Relevant parts of handle_rhs_call are probably
>
> /* As we compute ESCAPED context-insensitive we do not gain
> any precision with just EAF_NOCLOBBER but not EAF_NOESCAPE
> set. The argument would still get clobbered through the
> escape solution. */
> if ((flags & EAF_NOCLOBBER)
> && (flags & (EAF_NOESCAPE | EAF_NODIRECTESCAPE)))
> {
> ...
>
> specifically lines
>
> if (!(flags & (EAF_NOESCAPE | EAF_DIRECT)))
> make_indirect_escape_constraint (tem);
>
> probably do not trigger because of the invalid modref analysis. I suggest
> to look at the early modref pass dump (it's after FRE but still applies
> to callers)
Yeah, that makes sense.
Minor correction: we get into second branch of handle_rhs_call():
else if (flags & (EAF_NOESCAPE | EAF_NODIRECTESCAPE))
I traced it through around and I agree it looks like an ipa-modref bug.
Mechanically ipa-modref does not handle `gimple_call_return_slot_opt_p()` and
assumes 'foo = bar() [return slot optimization]' never escape 'foo'.
As a workaround I attempted to pessimize modref and it fixes the test case:
```diff
--- a/gcc/ipa-modref.c
+++ b/gcc/ipa-modref.c
@@ -1614,23 +1614,26 @@ analyze_ssa_name_flags (tree name, vec<modref_lattice>
&lattice, int depth,
{
if (gimple_return_retval (ret) == name)
lattice[index].merge (~EAF_UNUSED);
else if (memory_access_to (gimple_return_retval (ret), name))
lattice[index].merge_direct_load ();
}
/* Account for LHS store, arg loads and flags from callee function. */
else if (gcall *call = dyn_cast <gcall *> (use_stmt))
{
tree callee = gimple_call_fndecl (call);
-
+ if (gimple_call_return_slot_opt_p (call)
+ && gimple_call_lhs (call) != NULL_TREE
+ && TREE_ADDRESSABLE (TREE_TYPE (gimple_call_lhs (call))))
+ lattice[index].merge (0);
/* Recursion would require bit of propagation; give up for now. */
- if (callee && !ipa && recursive_call_p (current_function_decl,
+ else if (callee && !ipa && recursive_call_p (current_function_decl,
callee))
lattice[index].merge (0);
else
{
int ecf_flags = gimple_call_flags (call);
bool ignore_stores = ignore_stores_p (current_function_decl,
ecf_flags);
bool ignore_retval = ignore_retval_p (current_function_decl,
ecf_flags);
```
Mechanically ESCAPE bit was lost in Importer::Importer at:
1. in "this->base_path = dir_name (); [r s o]" ipa-modref derived 'DIRECT
NODIRECTESCAPE NOESCAPE' flags as it assumed it's just a memory store without
'this' escape.
2. main() optimised Inporter::Importer(&imp) as a noescape using
handle_rhs_call()
-> gimple_call_arg_flags(arg_index = 0)
-> - fnspec was empty
- modref's get_modref_function_summary() adds 'DIRECT NODIRECTESCAPE
NOESCAPE'
Is it a reasonable fix? Or it's too conservative and we could easily do better?
I copied predicate from handle_rhs_call(), but did not pick constrain copying:
/* And if we applied NRV the address of the return slot escapes as well. */
if (gimple_call_return_slot_opt_p (stmt)
&& gimple_call_lhs (stmt) != NULL_TREE
&& TREE_ADDRESSABLE (TREE_TYPE (gimple_call_lhs (stmt))))
{
auto_vec<ce_s> tmpc;
struct constraint_expr lhsc, *c;
get_constraint_for_address_of (gimple_call_lhs (stmt), &tmpc);
lhsc.var = escaped_id;
lhsc.offset = 0;
lhsc.type = SCALAR;
FOR_EACH_VEC_ELT (tmpc, i, c)
process_constraint (new_constraint (lhsc, *c));
}
Would constraint copy transfer to ipa-modref framework roughly the same?
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (8 preceding siblings ...)
2021-01-10 18:39 ` slyfox at gcc dot gnu.org
@ 2021-01-28 10:55 ` hubicka at gcc dot gnu.org
2021-01-30 18:02 ` slyfox at gcc dot gnu.org
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: hubicka at gcc dot gnu.org @ 2021-01-28 10:55 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
Jan Hubicka <hubicka at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Assignee|unassigned at gcc dot gnu.org |hubicka at gcc dot gnu.org
CC| |hubicka at gcc dot gnu.org
Status|NEW |ASSIGNED
--- Comment #9 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Thanks for all the detailed analysis and sorry for getting into this late.
> Oh, thank you! Only after many printf() attempts it sunk in that `036t.ealias` is using data from seemingly later `043t.modref1` pass. It is so confusing!
This is because it is an inter-procedural analysis. We compile in topological
order and propagate info from function to callers.
Here I think poblem is:
void Importer::Importer (struct Importer * const this)
{
struct string * _1;
<bb 2> :
*this_3(D) ={v} {CLOBBER};
*this_3(D).base_path = dir_name (); [return slot optimization]
return;
}
We get parm 0 flags: direct noescape nodirectescape
While dir_name does:
struct string dir_name ()
{
<bb 2> :
string::string (_2(D));
return _2(D);
}
and that gets to
void string::string (struct string * const this)
{
char[16] * _1;
<bb 2> :
*this_3(D) ={v} {CLOBBER};
_1 = &this_3(D)->_M_local_buf;
*this_3(D)._M_buf = _1;
return;
}
which indeed conflict with noescape.
So problem here is that return slot optimized variables are behaving kind of
like parameters. Since modref does not track EAF flags for them I think your
conservative fix makes sense.
It is also relatively easy to track the EAF flags here, I will try to get quick
stats on how often this makes difference (and whether we want to add trakcing
now or next stage1).
Honza
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (9 preceding siblings ...)
2021-01-28 10:55 ` hubicka at gcc dot gnu.org
@ 2021-01-30 18:02 ` slyfox at gcc dot gnu.org
2021-02-01 18:14 ` cvs-commit at gcc dot gnu.org
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-01-30 18:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #10 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #9)
> Thanks for all the detailed analysis and sorry for getting into this late.
>
> > Oh, thank you! Only after many printf() attempts it sunk in that `036t.ealias` is using data from seemingly later `043t.modref1` pass. It is so confusing!
>
> This is because it is an inter-procedural analysis. We compile in
> topological order and propagate info from function to callers.
>
> Here I think poblem is:
>
> void Importer::Importer (struct Importer * const this)
>
> {
>
> struct string * _1;
>
>
>
> <bb 2> :
>
> *this_3(D) ={v} {CLOBBER};
>
> *this_3(D).base_path = dir_name (); [return slot optimization]
>
> return;
>
>
>
> }
>
>
> We get parm 0 flags: direct noescape nodirectescape
>
> While dir_name does:
>
> struct string dir_name ()
>
> {
>
> <bb 2> :
>
> string::string (_2(D));
>
> return _2(D);
>
>
>
> }
> and that gets to
>
> void string::string (struct string * const this)
>
> {
>
> char[16] * _1;
>
>
>
> <bb 2> :
>
> *this_3(D) ={v} {CLOBBER};
>
> _1 = &this_3(D)->_M_local_buf;
>
> *this_3(D)._M_buf = _1;
>
> return;
>
>
>
> }
>
> which indeed conflict with noescape.
>
> So problem here is that return slot optimized variables are behaving kind of
> like parameters. Since modref does not track EAF flags for them I think
> your conservative fix makes sense.
>
> It is also relatively easy to track the EAF flags here, I will try to get
> quick stats on how often this makes difference (and whether we want to add
> trakcing now or next stage1).
Sounds great!
I send a conservative variant of the patch with the test as
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564579.html. I hope I
will not interfere with your possible improvement.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (10 preceding siblings ...)
2021-01-30 18:02 ` slyfox at gcc dot gnu.org
@ 2021-02-01 18:14 ` cvs-commit at gcc dot gnu.org
2021-02-01 18:39 ` slyfox at gcc dot gnu.org
2021-02-01 18:40 ` slyfox at gcc dot gnu.org
13 siblings, 0 replies; 15+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-02-01 18:14 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Sergei Trofimovich <slyfox@gcc.gnu.org>:
https://gcc.gnu.org/g:11056ab7687f7156846e93557c9171b77713bd7e
commit r11-7022-g11056ab7687f7156846e93557c9171b77713bd7e
Author: Sergei Trofimovich <siarheit@google.com>
Date: Mon Jan 11 18:05:57 2021 +0000
tree-optimization/98499 - fix modref analysis on RVO statements
Before the change RVO gimple statements were treated as local
stores by modres analysis. But in practice RVO escapes target.
2021-02-01 Sergei Trofimovich <siarheit@google.com>
gcc/ChangeLog:
PR tree-optimization/98499
* ipa-modref.c (analyze_ssa_name_flags): treat RVO
conservatively and assume all possible side-effects.
gcc/testsuite/ChangeLog:
PR tree-optimization/98499
* g++.dg/pr98499.C: new test.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (11 preceding siblings ...)
2021-02-01 18:14 ` cvs-commit at gcc dot gnu.org
@ 2021-02-01 18:39 ` slyfox at gcc dot gnu.org
2021-02-01 18:40 ` slyfox at gcc dot gnu.org
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-02-01 18:39 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
--- Comment #12 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
Should be fixed in gcc-11.
(In reply to Jan Hubicka from comment #9)
> It is also relatively easy to track the EAF flags here, I will try to get
> quick stats on how often this makes difference (and whether we want to add
> trakcing now or next stage1).
I filed a PR98925 to track the optimization.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug tree-optimization/98499] [11 Regression] Possibly bad std::string initialization in constructors
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
` (12 preceding siblings ...)
2021-02-01 18:39 ` slyfox at gcc dot gnu.org
@ 2021-02-01 18:40 ` slyfox at gcc dot gnu.org
13 siblings, 0 replies; 15+ messages in thread
From: slyfox at gcc dot gnu.org @ 2021-02-01 18:40 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98499
Sergei Trofimovich <slyfox at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|ASSIGNED |RESOLVED
--- Comment #13 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
Should be fixed in gcc-11.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-01 18:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-02 11:11 [Bug c++/98499] New: [11 Regression] Possibly bad std::string initialization in constructors slyfox at gcc dot gnu.org
2021-01-02 22:22 ` [Bug c++/98499] " slyfox at gcc dot gnu.org
2021-01-03 11:44 ` slyfox at gcc dot gnu.org
2021-01-03 20:56 ` slyfox at gcc dot gnu.org
2021-01-03 21:50 ` slyfox at gcc dot gnu.org
2021-01-04 12:28 ` marxin at gcc dot gnu.org
2021-01-05 11:09 ` rguenth at gcc dot gnu.org
2021-01-06 23:11 ` [Bug tree-optimization/98499] " slyfox at gcc dot gnu.org
2021-01-07 8:12 ` rguenth at gcc dot gnu.org
2021-01-10 18:39 ` slyfox at gcc dot gnu.org
2021-01-28 10:55 ` hubicka at gcc dot gnu.org
2021-01-30 18:02 ` slyfox at gcc dot gnu.org
2021-02-01 18:14 ` cvs-commit at gcc dot gnu.org
2021-02-01 18:39 ` slyfox at gcc dot gnu.org
2021-02-01 18:40 ` slyfox at gcc dot gnu.org
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).