* [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
@ 2018-01-04 21:49 David Malcolm
2018-01-05 9:36 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2018-01-04 21:49 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
PR lto/83121 reports an ICE deep inside the linemap code when -Wodr
reports on a type mismatch.
The root cause is that the warning can access the DECL_SOURCE_LOCATION
of a streamed-in decl before the lto_location_cache has been applied.
lto_location_cache::input_location stores RESERVED_LOCATION_COUNT (==2)
as a poison value until the cache is applied:
250 /* Keep value RESERVED_LOCATION_COUNT in *loc as linemap lookups will
251 ICE on it. */
The fix is relatively simple: apply the cache before reading the
DECL_SOURCE_LOCATION.
(I wonder if we should instead have a INVALID_LOCATION value to handle
this case more explicitly? e.g. 0xffffffff? or reserve 2 in libcpp for
that purpose, and have the non-reserved locations start at 3? Either
would be more invasive, though)
Triggering the ICE was fiddly: it seems to be affected by many things,
including the order of files, and (I think) by filenames. My theory is
that it's affected by the ordering of the tree nodes in the LTO stream:
for the ICE to occur, the types in question need to be compared before
some other operation flushes the lto_location_cache. This ordering
is affected by the hash-based ordering in DFS in lto-streamer-out.c, which
might explain why r255066 seemed to trigger the bug; the only relevant
change to LTO there seemed to be:
* lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and DECL_PADDING_P.
If so, then the bug was presumably already present, but hidden.
The patch also adds regression test coverage for the ICE, which is more
involved - as far as I can tell, we don't have an existing way to verify
diagnostics emitted during link-time optimization.
Hence the patch adds some machinery to lib/lto.exp to support two new
directives: dg-lto-warning and dg-lto-message, corresponding to
dg-warning and dg-message respectively, where the diagnostics are
expected to be emitted at link-time.
The test case includes examples of LTO warnings and notes in both the
primary and secondary source files
Doing so required reusing the logic from DejaGnu for handling diagnostics.
Unfortunately the pertinent code is a 50 line loop within a ~200 line Tcl
function in dg.exp (dg-test), so I had to copy it from DejaGnu, making
various changes as necessary (see lto_handle_diagnostics_for_file in the
patch; for example the LTO version supports multiple source files,
identifying which source file emitted a diagnostic).
For non-LTO diagnostics we currently ignore surplus "note" diagnostics.
This patch updates lto_prune_warns to follow this behavior (since
otherwise we'd need numerous dg-lto-message directives for the motivating
test case).
The patch adds these PASS results to g++.sum:
PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line 6)
PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line 8)
PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line 2)
PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line 3)
PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o link, -O0 -flto
The output for dg-lto-message above refers to "warnings", rather than
"messages" but that's the same as for the non-LTO case, where dg-message
also refers to "warnings".
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
OK for trunk?
gcc/ChangeLog:
PR lto/83121
* ipa-devirt.c (add_type_duplicate): When comparing memory layout,
call the lto_location_cache before reading the
DECL_SOURCE_LOCATION of the types.
gcc/testsuite/ChangeLog:
PR lto/83121
* g++.dg/lto/pr83121_0.C: New test case.
* g++.dg/lto/pr83121_1.C: New test case.
* lib/lto.exp (lto_handle_diagnostics_for_file): New procedure,
adapted from DejaGnu's dg-test.
(lto_handle_diagnostics): New procedure.
(lto_prune_warns): Ignore informational notes.
(lto-link-and-maybe-run): Add "messages_by_file" param.
Call lto_handle_diagnostics. Avoid issuing "unresolved" for
"execute" when "link" fails if "execute" was not specified.
(lto-can-handle-directive): New procedure.
(lto-get-options-main): Call lto-can-handle-directive. Add a
dg-messages local, using it to set the caller's
dg-messages-by-file for the given source file.
(lto-get-options): Likewise.
(lto-execute): Add dg-messages-by-file local, and pass it to
lto-link-and-maybe-run.
---
gcc/ipa-devirt.c | 7 +-
gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
gcc/testsuite/lib/lto.exp | 199 ++++++++++++++++++++++++++++++++++-
4 files changed, 222 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 540f038..f3d2e4a 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree type)
}
}
- /* Next compare memory layout. */
+ /* Next compare memory layout.
+ The DECL_SOURCE_LOCATIONs in this invocation came from LTO streaming.
+ We must apply the location cache to ensure that they are valid
+ before we can pass them to odr_types_equivalent_p (PR lto/83121). */
+ if (lto_location_cache::current_cache)
+ lto_location_cache::current_cache->apply_location_cache ();
if (!odr_types_equivalent_p (val->type, type,
!flag_ltrans && !val->odr_violated && !warned,
&warned, &visited,
diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C b/gcc/testsuite/g++.dg/lto/pr83121_0.C
new file mode 100644
index 0000000..ef894c7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
@@ -0,0 +1,12 @@
+// { dg-lto-do link }
+// { dg-lto-options {{-O0 -flto}} }
+/* We need -O0 to avoid the "Environment" locals in the test functions
+ from being optimized away. */
+
+struct Environment { // { dg-lto-warning "8: type 'struct Environment' violates the C\\+\\+ One Definition Rule" }
+ struct AsyncHooks {
+ int providers_[2]; // { dg-lto-message "a field of same name but different type is defined in another translation unit" }
+ };
+ AsyncHooks async_hooks_;
+};
+void fn2() { Environment a; }
diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C b/gcc/testsuite/g++.dg/lto/pr83121_1.C
new file mode 100644
index 0000000..2aef1b5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
@@ -0,0 +1,10 @@
+struct Environment {
+ struct AsyncHooks { // { dg-lto-warning "10: type 'struct AsyncHooks' violates the C\\+\\+ One Definition Rule" }
+ int providers_[1]; // { dg-lto-message "the first difference of corresponding definitions is field 'providers_'" }
+ };
+ AsyncHooks async_hooks_;
+};
+void fn1() { Environment a; }
+int main ()
+{
+}
diff --git a/gcc/testsuite/lib/lto.exp b/gcc/testsuite/lib/lto.exp
index 477f683..e69d8d4 100644
--- a/gcc/testsuite/lib/lto.exp
+++ b/gcc/testsuite/lib/lto.exp
@@ -16,6 +16,122 @@
# Contributed by Diego Novillo <dnovillo@google.com>
+# A subroutine of lto_handle_diagnostics: check TEXT for the expected
+# diagnostics for one specific source file, issuing PASS/FAIL results.
+# Return TEXT, stripped of any diagnostics that were handled.
+#
+# NAME is the testcase name to use when reporting PASS/FAIL results.
+# FILENAME is the name (with full path) of the file we're interested in.
+# MESSAGES_FOR_FILE is a list of expected messages, akin to DejaGnu's
+# "dg-messages" variable.
+# TEXT is the textual output from the LTO link.
+
+proc lto_handle_diagnostics_for_file { name filename messages_for_file text } {
+ global dg-linenum-format
+
+ set filename_without_path [file tail $filename]
+
+ # This loop is adapted from the related part of DejaGnu's dg-test,
+ # with changes as detailed below to cope with the LTO case.
+
+ foreach i ${messages_for_file} {
+ verbose "Scanning for message: $i" 4
+
+ # Remove all error messages for the line [lindex $i 0]
+ # in the source file. If we find any, success!
+ set line [lindex $i 0]
+ set pattern [lindex $i 2]
+ set comment [lindex $i 3]
+ verbose "line: $line" 4
+ verbose "pattern: $pattern" 4
+ verbose "comment: $comment" 4
+ #send_user "Before:\n$text\n"
+
+ # Unlike dg-test, we use $filename_without_path in this pattern.
+ # This is to ensure that we have the correct file/line combination.
+ # This imposes the restriction that the filename can't contain
+ # any regexp control characters. We have to strip the path, since
+ # e.g. the '+' in "g++.dg" wouldn't be valid.
+ set pat "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[^\n\]*\n?)+"
+ if {[regsub -all $pat $text "\n" text]} {
+ set text [string trimleft $text]
+ set ok pass
+ set uhoh fail
+ } else {
+ set ok fail
+ set uhoh pass
+ }
+ #send_user "After:\n$text\n"
+
+ # $line will either be a formatted line number or a number all by
+ # itself. Delete the formatting.
+ scan $line ${dg-linenum-format} line
+
+ # Unlike dg-test, add the filename to the PASS/FAIL message (rather
+ # than just the line number) so that the user can identify the
+ # pertinent directive.
+ set describe_where "$filename_without_path line $line"
+
+ # Issue the PASS/FAIL, adding "LTO" to the messages (e.g. "LTO errors")
+ # to distinguish them from the non-LTO case (in case we ever need to
+ # support both).
+ switch [lindex $i 1] {
+ "ERROR" {
+ $ok "$name $comment (test for LTO errors, $describe_where)"
+ }
+ "XERROR" {
+ x$ok "$name $comment (test for LTO errors, $describe_where)"
+ }
+ "WARNING" {
+ $ok "$name $comment (test for LTO warnings, $describe_where)"
+ }
+ "XWARNING" {
+ x$ok "$name $comment (test for LTO warnings, $describe_where)"
+ }
+ "BOGUS" {
+ $uhoh "$name $comment (test for LTO bogus messages, $describe_where)"
+ }
+ "XBOGUS" {
+ x$uhoh "$name $comment (test for LTO bogus messages, $describe_where)"
+ }
+ "BUILD" {
+ $uhoh "$name $comment (test for LTO build failure, $describe_where)"
+ }
+ "XBUILD" {
+ x$uhoh "$name $comment (test for LTO build failure, $describe_where)"
+ }
+ "EXEC" { }
+ "XEXEC" { }
+ }
+ }
+ return $text
+}
+
+# Support for checking for link-time diagnostics: check for
+# the expected diagnostics within TEXT, issuing PASS/FAIL results.
+# Return TEXT, stripped of any diagnostics that were handled.
+#
+# MESSAGES_BY_FILE is a dict; the keys are source files (with paths)
+# the values are lists of expected messages, akin to DejaGnu's "dg-messages"
+# variable.
+# TEXT is the textual output from the LTO link.
+
+proc lto_handle_diagnostics { messages_by_file text } {
+ global testcase
+
+ verbose "lto_handle_diagnostics: entry: $text" 2
+ verbose " messages_by_file $messages_by_file" 3
+
+ dict for {src dg-messages} $messages_by_file {
+ set text [lto_handle_diagnostics_for_file $testcase $src \
+ ${dg-messages} $text]
+ }
+
+ verbose "lto_handle_diagnostics: exit: $text" 2
+
+ return $text
+}
+
# Prune messages that aren't useful.
proc lto_prune_warns { text } {
@@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]* value=\[^\n\]*; file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken" $text "" text
+ # Ignore informational notes.
+ regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
+
verbose "lto_prune_warns: exit: $text" 2
return $text
@@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile optstr xfaildata } {
# OPTALL is a list of compiler and linker options to use for all tests
# OPTFILE is a list of compiler and linker options to use for this test
# OPTSTR is the list of options to list in messages
-proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr } {
+# MESSAGES_BY_FILE is a dict of (src, dg-messages)
+proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr \
+ messages_by_file } {
global testcase
global tool
global compile_type
global board_info
+ verbose "lto-link-and-maybe-run" 2
+ verbose " messages_by_file $messages_by_file" 3
+
# Check that all of the objects were built successfully.
foreach obj [split $objlist] {
if ![file_on_host exists $obj] then {
@@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr } {
set board_info($target_board,ldscript) $saved_ldscript
}
+ # Check for diagnostics specified by directives
+ set comp_output [lto_handle_diagnostics $messages_by_file $comp_output]
+
# Prune unimportant visibility warnings before checking output.
set comp_output [lto_prune_warns $comp_output]
if ![${tool}_check_compile "$testcase $testname link" $optstr \
$dest $comp_output] then {
- unresolved "$testcase $testname execute $optstr"
+ if { ![string compare "execute" $compile_type] } {
+ unresolved "$testcase $testname execute $optstr"
+ }
return
}
@@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr } {
$status "$testcase $testname execute $optstr"
}
+# Potentially handle the given dg- directive (a list)
+# Return true is the directive was handled, false otherwise.
+
+proc lto-can-handle-directive { op } {
+ set cmd [lindex $op 0]
+
+ # dg-warning and dg-message append to dg-messages.
+ upvar dg-messages dg-messages
+
+ # A list of directives to recognize, and a list of directives
+ # to remap them to.
+ # For example, "dg-lto-warning" is implemented by calling "dg-warning".
+ set directives { dg-lto-warning dg-lto-message }
+ set remapped_directives { dg-warning dg-message }
+
+ set idx [lsearch -exact $directives $cmd]
+ if { $idx != -1 } {
+ verbose "remapping from: $op" 4
+
+ set remapped_cmd [lindex $remapped_directives $idx]
+ set op [lreplace $op 0 0 $remapped_cmd]
+
+ verbose "remapped to: $op" 4
+
+ set status [catch "$op" errmsg]
+ if { $status != 0 } {
+ if { 0 && [info exists errorInfo] } {
+ # This also prints a backtrace which will just confuse
+ # testcase writers, so it's disabled.
+ perror "$name: $errorInfo\n"
+ } else {
+ perror "$name: $errmsg for \"$op\"\n"
+ }
+ # ??? The call to unresolved here is necessary to clear `errcnt'.
+ # What we really need is a proc like perror that doesn't set errcnt.
+ # It should also set exit_status to 1.
+ unresolved "$name: $errmsg for \"$op\""
+ }
+
+ return true
+ }
+
+ return false
+}
+
# lto-get-options-main -- get target requirements for a test and
# options for the primary source file and the test as a whole
#
@@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
upvar dg-final-code dg-final-code
set dg-final-code ""
+ # dg-warning and dg-message append to dg-messages.
+ upvar dg-messages-by-file dg-messages-by-file
+ set dg-messages ""
+
set tmp [dg-get-options $src]
verbose "getting options for $src: $tmp"
foreach op $tmp {
@@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
} else {
append dg-final-code "[lindex $op 2]\n"
}
- } else {
+ } elseif { ![lto-can-handle-directive $op] } {
# Ignore unrecognized dg- commands, but warn about them.
warning "lto.exp does not support $cmd"
}
}
+ verbose "dg-messages: ${dg-messages}" 3
+ dict append dg-messages-by-file $src ${dg-messages}
+
# Return flags to use for compiling the primary source file and for
# linking.
verbose "dg-extra-tool-flags for main is ${dg-extra-tool-flags}"
@@ -373,6 +554,10 @@ proc lto-get-options { src } {
# dg-xfail-if needs access to dg-do-what.
upvar dg-do-what dg-do-what
+ # dg-warning appends to dg-messages.
+ upvar dg-messages-by-file dg-messages-by-file
+ set dg-messages ""
+
set tmp [dg-get-options $src]
foreach op $tmp {
set cmd [lindex $op 0]
@@ -386,12 +571,15 @@ proc lto-get-options { src } {
}
} elseif { [string match "dg-require-*" $cmd] } {
warning "lto.exp does not support $cmd in secondary source files"
- } else {
+ } elseif { ![lto-can-handle-directive $op] } {
# Ignore unrecognized dg- commands, but warn about them.
warning "lto.exp does not support $cmd in secondary source files"
}
}
+ verbose "dg-messages: ${dg-messages}" 3
+ dict append dg-messages-by-file $src ${dg-messages}
+
return ${dg-extra-tool-flags}
}
@@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
verbose "lto-execute: $src1" 1
set compile_type "run"
set dg-do-what [list ${dg-do-what-default} "" P]
+ set dg-messages-by-file [dict create]
set extra_flags(0) [lto-get-options-main $src1]
set compile_xfail(0) ""
@@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
lto-link-and-maybe-run \
"[lindex $obj_list 0]-[lindex $obj_list end]" \
$obj_list $execname $filtered ${dg-extra-ld-options} \
- $filtered
+ $filtered ${dg-messages-by-file}
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
2018-01-04 21:49 [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121) David Malcolm
@ 2018-01-05 9:36 ` Richard Biener
2018-01-05 22:55 ` David Malcolm
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2018-01-05 9:36 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches
On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> PR lto/83121 reports an ICE deep inside the linemap code when -Wodr
> reports on a type mismatch.
>
> The root cause is that the warning can access the DECL_SOURCE_LOCATION
> of a streamed-in decl before the lto_location_cache has been applied.
>
> lto_location_cache::input_location stores RESERVED_LOCATION_COUNT (==2)
> as a poison value until the cache is applied:
> 250 /* Keep value RESERVED_LOCATION_COUNT in *loc as linemap lookups will
> 251 ICE on it. */
>
> The fix is relatively simple: apply the cache before reading the
> DECL_SOURCE_LOCATION.
>
> (I wonder if we should instead have a INVALID_LOCATION value to handle
> this case more explicitly? e.g. 0xffffffff? or reserve 2 in libcpp for
> that purpose, and have the non-reserved locations start at 3? Either
> would be more invasive, though)
>
> Triggering the ICE was fiddly: it seems to be affected by many things,
> including the order of files, and (I think) by filenames. My theory is
> that it's affected by the ordering of the tree nodes in the LTO stream:
> for the ICE to occur, the types in question need to be compared before
> some other operation flushes the lto_location_cache. This ordering
> is affected by the hash-based ordering in DFS in lto-streamer-out.c, which
> might explain why r255066 seemed to trigger the bug; the only relevant
> change to LTO there seemed to be:
> * lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and DECL_PADDING_P.
> If so, then the bug was presumably already present, but hidden.
>
> The patch also adds regression test coverage for the ICE, which is more
> involved - as far as I can tell, we don't have an existing way to verify
> diagnostics emitted during link-time optimization.
>
> Hence the patch adds some machinery to lib/lto.exp to support two new
> directives: dg-lto-warning and dg-lto-message, corresponding to
> dg-warning and dg-message respectively, where the diagnostics are
> expected to be emitted at link-time.
>
> The test case includes examples of LTO warnings and notes in both the
> primary and secondary source files
>
> Doing so required reusing the logic from DejaGnu for handling diagnostics.
> Unfortunately the pertinent code is a 50 line loop within a ~200 line Tcl
> function in dg.exp (dg-test), so I had to copy it from DejaGnu, making
> various changes as necessary (see lto_handle_diagnostics_for_file in the
> patch; for example the LTO version supports multiple source files,
> identifying which source file emitted a diagnostic).
>
> For non-LTO diagnostics we currently ignore surplus "note" diagnostics.
> This patch updates lto_prune_warns to follow this behavior (since
> otherwise we'd need numerous dg-lto-message directives for the motivating
> test case).
>
> The patch adds these PASS results to g++.sum:
>
> PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
> PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
> PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line 6)
> PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line 8)
> PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line 2)
> PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line 3)
> PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o link, -O0 -flto
>
> The output for dg-lto-message above refers to "warnings", rather than
> "messages" but that's the same as for the non-LTO case, where dg-message
> also refers to "warnings".
>
> Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
Hmm, but we do this in warn_odr already? How's that not enough?
At least it seems the place you add this isn't ideal (not at the "root cause").
Richard.
> gcc/ChangeLog:
> PR lto/83121
> * ipa-devirt.c (add_type_duplicate): When comparing memory layout,
> call the lto_location_cache before reading the
> DECL_SOURCE_LOCATION of the types.
>
> gcc/testsuite/ChangeLog:
> PR lto/83121
> * g++.dg/lto/pr83121_0.C: New test case.
> * g++.dg/lto/pr83121_1.C: New test case.
> * lib/lto.exp (lto_handle_diagnostics_for_file): New procedure,
> adapted from DejaGnu's dg-test.
> (lto_handle_diagnostics): New procedure.
> (lto_prune_warns): Ignore informational notes.
> (lto-link-and-maybe-run): Add "messages_by_file" param.
> Call lto_handle_diagnostics. Avoid issuing "unresolved" for
> "execute" when "link" fails if "execute" was not specified.
> (lto-can-handle-directive): New procedure.
> (lto-get-options-main): Call lto-can-handle-directive. Add a
> dg-messages local, using it to set the caller's
> dg-messages-by-file for the given source file.
> (lto-get-options): Likewise.
> (lto-execute): Add dg-messages-by-file local, and pass it to
> lto-link-and-maybe-run.
> ---
> gcc/ipa-devirt.c | 7 +-
> gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
> gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
> gcc/testsuite/lib/lto.exp | 199 ++++++++++++++++++++++++++++++++++-
> 4 files changed, 222 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
> create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
>
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 540f038..f3d2e4a 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree type)
> }
> }
>
> - /* Next compare memory layout. */
> + /* Next compare memory layout.
> + The DECL_SOURCE_LOCATIONs in this invocation came from LTO streaming.
> + We must apply the location cache to ensure that they are valid
> + before we can pass them to odr_types_equivalent_p (PR lto/83121). */
> + if (lto_location_cache::current_cache)
> + lto_location_cache::current_cache->apply_location_cache ();
> if (!odr_types_equivalent_p (val->type, type,
> !flag_ltrans && !val->odr_violated && !warned,
> &warned, &visited,
> diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C b/gcc/testsuite/g++.dg/lto/pr83121_0.C
> new file mode 100644
> index 0000000..ef894c7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
> @@ -0,0 +1,12 @@
> +// { dg-lto-do link }
> +// { dg-lto-options {{-O0 -flto}} }
> +/* We need -O0 to avoid the "Environment" locals in the test functions
> + from being optimized away. */
> +
> +struct Environment { // { dg-lto-warning "8: type 'struct Environment' violates the C\\+\\+ One Definition Rule" }
> + struct AsyncHooks {
> + int providers_[2]; // { dg-lto-message "a field of same name but different type is defined in another translation unit" }
> + };
> + AsyncHooks async_hooks_;
> +};
> +void fn2() { Environment a; }
> diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C b/gcc/testsuite/g++.dg/lto/pr83121_1.C
> new file mode 100644
> index 0000000..2aef1b5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
> @@ -0,0 +1,10 @@
> +struct Environment {
> + struct AsyncHooks { // { dg-lto-warning "10: type 'struct AsyncHooks' violates the C\\+\\+ One Definition Rule" }
> + int providers_[1]; // { dg-lto-message "the first difference of corresponding definitions is field 'providers_'" }
> + };
> + AsyncHooks async_hooks_;
> +};
> +void fn1() { Environment a; }
> +int main ()
> +{
> +}
> diff --git a/gcc/testsuite/lib/lto.exp b/gcc/testsuite/lib/lto.exp
> index 477f683..e69d8d4 100644
> --- a/gcc/testsuite/lib/lto.exp
> +++ b/gcc/testsuite/lib/lto.exp
> @@ -16,6 +16,122 @@
>
> # Contributed by Diego Novillo <dnovillo@google.com>
>
> +# A subroutine of lto_handle_diagnostics: check TEXT for the expected
> +# diagnostics for one specific source file, issuing PASS/FAIL results.
> +# Return TEXT, stripped of any diagnostics that were handled.
> +#
> +# NAME is the testcase name to use when reporting PASS/FAIL results.
> +# FILENAME is the name (with full path) of the file we're interested in.
> +# MESSAGES_FOR_FILE is a list of expected messages, akin to DejaGnu's
> +# "dg-messages" variable.
> +# TEXT is the textual output from the LTO link.
> +
> +proc lto_handle_diagnostics_for_file { name filename messages_for_file text } {
> + global dg-linenum-format
> +
> + set filename_without_path [file tail $filename]
> +
> + # This loop is adapted from the related part of DejaGnu's dg-test,
> + # with changes as detailed below to cope with the LTO case.
> +
> + foreach i ${messages_for_file} {
> + verbose "Scanning for message: $i" 4
> +
> + # Remove all error messages for the line [lindex $i 0]
> + # in the source file. If we find any, success!
> + set line [lindex $i 0]
> + set pattern [lindex $i 2]
> + set comment [lindex $i 3]
> + verbose "line: $line" 4
> + verbose "pattern: $pattern" 4
> + verbose "comment: $comment" 4
> + #send_user "Before:\n$text\n"
> +
> + # Unlike dg-test, we use $filename_without_path in this pattern.
> + # This is to ensure that we have the correct file/line combination.
> + # This imposes the restriction that the filename can't contain
> + # any regexp control characters. We have to strip the path, since
> + # e.g. the '+' in "g++.dg" wouldn't be valid.
> + set pat "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[^\n\]*\n?)+"
> + if {[regsub -all $pat $text "\n" text]} {
> + set text [string trimleft $text]
> + set ok pass
> + set uhoh fail
> + } else {
> + set ok fail
> + set uhoh pass
> + }
> + #send_user "After:\n$text\n"
> +
> + # $line will either be a formatted line number or a number all by
> + # itself. Delete the formatting.
> + scan $line ${dg-linenum-format} line
> +
> + # Unlike dg-test, add the filename to the PASS/FAIL message (rather
> + # than just the line number) so that the user can identify the
> + # pertinent directive.
> + set describe_where "$filename_without_path line $line"
> +
> + # Issue the PASS/FAIL, adding "LTO" to the messages (e.g. "LTO errors")
> + # to distinguish them from the non-LTO case (in case we ever need to
> + # support both).
> + switch [lindex $i 1] {
> + "ERROR" {
> + $ok "$name $comment (test for LTO errors, $describe_where)"
> + }
> + "XERROR" {
> + x$ok "$name $comment (test for LTO errors, $describe_where)"
> + }
> + "WARNING" {
> + $ok "$name $comment (test for LTO warnings, $describe_where)"
> + }
> + "XWARNING" {
> + x$ok "$name $comment (test for LTO warnings, $describe_where)"
> + }
> + "BOGUS" {
> + $uhoh "$name $comment (test for LTO bogus messages, $describe_where)"
> + }
> + "XBOGUS" {
> + x$uhoh "$name $comment (test for LTO bogus messages, $describe_where)"
> + }
> + "BUILD" {
> + $uhoh "$name $comment (test for LTO build failure, $describe_where)"
> + }
> + "XBUILD" {
> + x$uhoh "$name $comment (test for LTO build failure, $describe_where)"
> + }
> + "EXEC" { }
> + "XEXEC" { }
> + }
> + }
> + return $text
> +}
> +
> +# Support for checking for link-time diagnostics: check for
> +# the expected diagnostics within TEXT, issuing PASS/FAIL results.
> +# Return TEXT, stripped of any diagnostics that were handled.
> +#
> +# MESSAGES_BY_FILE is a dict; the keys are source files (with paths)
> +# the values are lists of expected messages, akin to DejaGnu's "dg-messages"
> +# variable.
> +# TEXT is the textual output from the LTO link.
> +
> +proc lto_handle_diagnostics { messages_by_file text } {
> + global testcase
> +
> + verbose "lto_handle_diagnostics: entry: $text" 2
> + verbose " messages_by_file $messages_by_file" 3
> +
> + dict for {src dg-messages} $messages_by_file {
> + set text [lto_handle_diagnostics_for_file $testcase $src \
> + ${dg-messages} $text]
> + }
> +
> + verbose "lto_handle_diagnostics: exit: $text" 2
> +
> + return $text
> +}
> +
> # Prune messages that aren't useful.
>
> proc lto_prune_warns { text } {
> @@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
> regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]* value=\[^\n\]*; file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
> regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken" $text "" text
>
> + # Ignore informational notes.
> + regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
> +
> verbose "lto_prune_warns: exit: $text" 2
>
> return $text
> @@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile optstr xfaildata } {
> # OPTALL is a list of compiler and linker options to use for all tests
> # OPTFILE is a list of compiler and linker options to use for this test
> # OPTSTR is the list of options to list in messages
> -proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr } {
> +# MESSAGES_BY_FILE is a dict of (src, dg-messages)
> +proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr \
> + messages_by_file } {
> global testcase
> global tool
> global compile_type
> global board_info
>
> + verbose "lto-link-and-maybe-run" 2
> + verbose " messages_by_file $messages_by_file" 3
> +
> # Check that all of the objects were built successfully.
> foreach obj [split $objlist] {
> if ![file_on_host exists $obj] then {
> @@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr } {
> set board_info($target_board,ldscript) $saved_ldscript
> }
>
> + # Check for diagnostics specified by directives
> + set comp_output [lto_handle_diagnostics $messages_by_file $comp_output]
> +
> # Prune unimportant visibility warnings before checking output.
> set comp_output [lto_prune_warns $comp_output]
>
> if ![${tool}_check_compile "$testcase $testname link" $optstr \
> $dest $comp_output] then {
> - unresolved "$testcase $testname execute $optstr"
> + if { ![string compare "execute" $compile_type] } {
> + unresolved "$testcase $testname execute $optstr"
> + }
> return
> }
>
> @@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname objlist dest optall optfile optstr } {
> $status "$testcase $testname execute $optstr"
> }
>
> +# Potentially handle the given dg- directive (a list)
> +# Return true is the directive was handled, false otherwise.
> +
> +proc lto-can-handle-directive { op } {
> + set cmd [lindex $op 0]
> +
> + # dg-warning and dg-message append to dg-messages.
> + upvar dg-messages dg-messages
> +
> + # A list of directives to recognize, and a list of directives
> + # to remap them to.
> + # For example, "dg-lto-warning" is implemented by calling "dg-warning".
> + set directives { dg-lto-warning dg-lto-message }
> + set remapped_directives { dg-warning dg-message }
> +
> + set idx [lsearch -exact $directives $cmd]
> + if { $idx != -1 } {
> + verbose "remapping from: $op" 4
> +
> + set remapped_cmd [lindex $remapped_directives $idx]
> + set op [lreplace $op 0 0 $remapped_cmd]
> +
> + verbose "remapped to: $op" 4
> +
> + set status [catch "$op" errmsg]
> + if { $status != 0 } {
> + if { 0 && [info exists errorInfo] } {
> + # This also prints a backtrace which will just confuse
> + # testcase writers, so it's disabled.
> + perror "$name: $errorInfo\n"
> + } else {
> + perror "$name: $errmsg for \"$op\"\n"
> + }
> + # ??? The call to unresolved here is necessary to clear `errcnt'.
> + # What we really need is a proc like perror that doesn't set errcnt.
> + # It should also set exit_status to 1.
> + unresolved "$name: $errmsg for \"$op\""
> + }
> +
> + return true
> + }
> +
> + return false
> +}
> +
> # lto-get-options-main -- get target requirements for a test and
> # options for the primary source file and the test as a whole
> #
> @@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
> upvar dg-final-code dg-final-code
> set dg-final-code ""
>
> + # dg-warning and dg-message append to dg-messages.
> + upvar dg-messages-by-file dg-messages-by-file
> + set dg-messages ""
> +
> set tmp [dg-get-options $src]
> verbose "getting options for $src: $tmp"
> foreach op $tmp {
> @@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
> } else {
> append dg-final-code "[lindex $op 2]\n"
> }
> - } else {
> + } elseif { ![lto-can-handle-directive $op] } {
> # Ignore unrecognized dg- commands, but warn about them.
> warning "lto.exp does not support $cmd"
> }
> }
>
> + verbose "dg-messages: ${dg-messages}" 3
> + dict append dg-messages-by-file $src ${dg-messages}
> +
> # Return flags to use for compiling the primary source file and for
> # linking.
> verbose "dg-extra-tool-flags for main is ${dg-extra-tool-flags}"
> @@ -373,6 +554,10 @@ proc lto-get-options { src } {
> # dg-xfail-if needs access to dg-do-what.
> upvar dg-do-what dg-do-what
>
> + # dg-warning appends to dg-messages.
> + upvar dg-messages-by-file dg-messages-by-file
> + set dg-messages ""
> +
> set tmp [dg-get-options $src]
> foreach op $tmp {
> set cmd [lindex $op 0]
> @@ -386,12 +571,15 @@ proc lto-get-options { src } {
> }
> } elseif { [string match "dg-require-*" $cmd] } {
> warning "lto.exp does not support $cmd in secondary source files"
> - } else {
> + } elseif { ![lto-can-handle-directive $op] } {
> # Ignore unrecognized dg- commands, but warn about them.
> warning "lto.exp does not support $cmd in secondary source files"
> }
> }
>
> + verbose "dg-messages: ${dg-messages}" 3
> + dict append dg-messages-by-file $src ${dg-messages}
> +
> return ${dg-extra-tool-flags}
> }
>
> @@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
> verbose "lto-execute: $src1" 1
> set compile_type "run"
> set dg-do-what [list ${dg-do-what-default} "" P]
> + set dg-messages-by-file [dict create]
> set extra_flags(0) [lto-get-options-main $src1]
> set compile_xfail(0) ""
>
> @@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
> lto-link-and-maybe-run \
> "[lindex $obj_list 0]-[lindex $obj_list end]" \
> $obj_list $execname $filtered ${dg-extra-ld-options} \
> - $filtered
> + $filtered ${dg-messages-by-file}
> }
>
>
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
2018-01-05 9:36 ` Richard Biener
@ 2018-01-05 22:55 ` David Malcolm
2018-01-06 7:44 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2018-01-05 22:55 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, Jan Hubicka
On Fri, 2018-01-05 at 10:36 +0100, Richard Biener wrote:
> On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > PR lto/83121 reports an ICE deep inside the linemap code when -Wodr
> > reports on a type mismatch.
> >
> > The root cause is that the warning can access the
> > DECL_SOURCE_LOCATION
> > of a streamed-in decl before the lto_location_cache has been
> > applied.
> >
> > lto_location_cache::input_location stores RESERVED_LOCATION_COUNT
> > (==2)
> > as a poison value until the cache is applied:
> > 250 /* Keep value RESERVED_LOCATION_COUNT in *loc as linemap
> > lookups will
> > 251 ICE on it. */
> >
> > The fix is relatively simple: apply the cache before reading the
> > DECL_SOURCE_LOCATION.
> >
> > (I wonder if we should instead have a INVALID_LOCATION value to
> > handle
> > this case more explicitly? e.g. 0xffffffff? or reserve 2 in
> > libcpp for
> > that purpose, and have the non-reserved locations start at
> > 3? Either
> > would be more invasive, though)
> >
> > Triggering the ICE was fiddly: it seems to be affected by many
> > things,
> > including the order of files, and (I think) by filenames. My
> > theory is
> > that it's affected by the ordering of the tree nodes in the LTO
> > stream:
> > for the ICE to occur, the types in question need to be compared
> > before
> > some other operation flushes the lto_location_cache. This ordering
> > is affected by the hash-based ordering in DFS in lto-streamer-
> > out.c, which
> > might explain why r255066 seemed to trigger the bug; the only
> > relevant
> > change to LTO there seemed to be:
> > * lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and
> > DECL_PADDING_P.
> > If so, then the bug was presumably already present, but hidden.
> >
> > The patch also adds regression test coverage for the ICE, which is
> > more
> > involved - as far as I can tell, we don't have an existing way to
> > verify
> > diagnostics emitted during link-time optimization.
> >
> > Hence the patch adds some machinery to lib/lto.exp to support two
> > new
> > directives: dg-lto-warning and dg-lto-message, corresponding to
> > dg-warning and dg-message respectively, where the diagnostics are
> > expected to be emitted at link-time.
> >
> > The test case includes examples of LTO warnings and notes in both
> > the
> > primary and secondary source files
> >
> > Doing so required reusing the logic from DejaGnu for handling
> > diagnostics.
> > Unfortunately the pertinent code is a 50 line loop within a ~200
> > line Tcl
> > function in dg.exp (dg-test), so I had to copy it from DejaGnu,
> > making
> > various changes as necessary (see lto_handle_diagnostics_for_file
> > in the
> > patch; for example the LTO version supports multiple source files,
> > identifying which source file emitted a diagnostic).
> >
> > For non-LTO diagnostics we currently ignore surplus "note"
> > diagnostics.
> > This patch updates lto_prune_warns to follow this behavior (since
> > otherwise we'd need numerous dg-lto-message directives for the
> > motivating
> > test case).
> >
> > The patch adds these PASS results to g++.sum:
> >
> > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
> > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line
> > 6)
> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line
> > 8)
> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line
> > 2)
> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line
> > 3)
> > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o
> > link, -O0 -flto
> >
> > The output for dg-lto-message above refers to "warnings", rather
> > than
> > "messages" but that's the same as for the non-LTO case, where dg-
> > message
> > also refers to "warnings".
> >
> > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
> >
> > OK for trunk?
>
> Hmm, but we do this in warn_odr already? How's that not enough?
>
> At least it seems the place you add this isn't ideal (not at the
> "root cause").
>
> Richard.
[CCing Honza]
Yes, warn_odr does apply the cache, but this warning is coming from
warn_types_mismatch.
Of the various calls to warn_types_mismatch, most are immediately after
a warn_odr guarded by if (warn && warned) or if (warned), and if
warn_odr writes back true to *warned, it has definitely applied the
cache.
However, the 7-argument overload of odr_types_equivalent_p can
also call warn_types_mismatch, passing in the location_t values it
received.
The problem occurs with this call stack, when:
add_type_duplicate, passes DECL_SOURCE_LOCATIONs to:
odr_types_equivalent_p (7-argument overload) - which calls:
warn_types_mismatch, which calls:
warning_at with the given location_t
where the DECL_SOURCE_LOCATION might not yet have been fixed up yet.
My patch adds the fixup there.
This behavior seems to have been introduced by r224248 (aka
379ca7f842e5b13109b689b3c732741cab3d178e), Honza's fix for "PR
lto/65378" (though that seems to have been a typo, as that bug is
unrelated to the changes in that commit); the relevant ML post was:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00523.html
Would you prefer this if I reworked the two-liner into something like
an "lto_location_cache::ensure_cache_applied" function?
Thanks
Dave
>
> > gcc/ChangeLog:
> > PR lto/83121
> > * ipa-devirt.c (add_type_duplicate): When comparing memory
> > layout,
> > call the lto_location_cache before reading the
> > DECL_SOURCE_LOCATION of the types.
> >
> > gcc/testsuite/ChangeLog:
> > PR lto/83121
> > * g++.dg/lto/pr83121_0.C: New test case.
> > * g++.dg/lto/pr83121_1.C: New test case.
> > * lib/lto.exp (lto_handle_diagnostics_for_file): New
> > procedure,
> > adapted from DejaGnu's dg-test.
> > (lto_handle_diagnostics): New procedure.
> > (lto_prune_warns): Ignore informational notes.
> > (lto-link-and-maybe-run): Add "messages_by_file" param.
> > Call lto_handle_diagnostics. Avoid issuing "unresolved"
> > for
> > "execute" when "link" fails if "execute" was not specified.
> > (lto-can-handle-directive): New procedure.
> > (lto-get-options-main): Call lto-can-handle-directive. Add
> > a
> > dg-messages local, using it to set the caller's
> > dg-messages-by-file for the given source file.
> > (lto-get-options): Likewise.
> > (lto-execute): Add dg-messages-by-file local, and pass it
> > to
> > lto-link-and-maybe-run.
> > ---
> > gcc/ipa-devirt.c | 7 +-
> > gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
> > gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
> > gcc/testsuite/lib/lto.exp | 199
> > ++++++++++++++++++++++++++++++++++-
> > 4 files changed, 222 insertions(+), 6 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
> > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
> >
> > diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> > index 540f038..f3d2e4a 100644
> > --- a/gcc/ipa-devirt.c
> > +++ b/gcc/ipa-devirt.c
> > @@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree type)
> > }
> > }
> >
> > - /* Next compare memory layout. */
> > + /* Next compare memory layout.
> > + The DECL_SOURCE_LOCATIONs in this invocation came from LTO
> > streaming.
> > + We must apply the location cache to ensure that they are
> > valid
> > + before we can pass them to odr_types_equivalent_p (PR
> > lto/83121). */
> > + if (lto_location_cache::current_cache)
> > + lto_location_cache::current_cache->apply_location_cache ();
> > if (!odr_types_equivalent_p (val->type, type,
> > !flag_ltrans && !val->odr_violated
> > && !warned,
> > &warned, &visited,
> > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C
> > b/gcc/testsuite/g++.dg/lto/pr83121_0.C
> > new file mode 100644
> > index 0000000..ef894c7
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
> > @@ -0,0 +1,12 @@
> > +// { dg-lto-do link }
> > +// { dg-lto-options {{-O0 -flto}} }
> > +/* We need -O0 to avoid the "Environment" locals in the test
> > functions
> > + from being optimized away. */
> > +
> > +struct Environment { // { dg-lto-warning "8: type 'struct
> > Environment' violates the C\\+\\+ One Definition Rule" }
> > + struct AsyncHooks {
> > + int providers_[2]; // { dg-lto-message "a field of same name
> > but different type is defined in another translation unit" }
> > + };
> > + AsyncHooks async_hooks_;
> > +};
> > +void fn2() { Environment a; }
> > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C
> > b/gcc/testsuite/g++.dg/lto/pr83121_1.C
> > new file mode 100644
> > index 0000000..2aef1b5
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
> > @@ -0,0 +1,10 @@
> > +struct Environment {
> > + struct AsyncHooks { // { dg-lto-warning "10: type 'struct
> > AsyncHooks' violates the C\\+\\+ One Definition Rule" }
> > + int providers_[1]; // { dg-lto-message "the first difference
> > of corresponding definitions is field 'providers_'" }
> > + };
> > + AsyncHooks async_hooks_;
> > +};
> > +void fn1() { Environment a; }
> > +int main ()
> > +{
> > +}
> > diff --git a/gcc/testsuite/lib/lto.exp b/gcc/testsuite/lib/lto.exp
> > index 477f683..e69d8d4 100644
> > --- a/gcc/testsuite/lib/lto.exp
> > +++ b/gcc/testsuite/lib/lto.exp
> > @@ -16,6 +16,122 @@
> >
> > # Contributed by Diego Novillo <dnovillo@google.com>
> >
> > +# A subroutine of lto_handle_diagnostics: check TEXT for the
> > expected
> > +# diagnostics for one specific source file, issuing PASS/FAIL
> > results.
> > +# Return TEXT, stripped of any diagnostics that were handled.
> > +#
> > +# NAME is the testcase name to use when reporting PASS/FAIL
> > results.
> > +# FILENAME is the name (with full path) of the file we're
> > interested in.
> > +# MESSAGES_FOR_FILE is a list of expected messages, akin to
> > DejaGnu's
> > +# "dg-messages" variable.
> > +# TEXT is the textual output from the LTO link.
> > +
> > +proc lto_handle_diagnostics_for_file { name filename
> > messages_for_file text } {
> > + global dg-linenum-format
> > +
> > + set filename_without_path [file tail $filename]
> > +
> > + # This loop is adapted from the related part of DejaGnu's dg-
> > test,
> > + # with changes as detailed below to cope with the LTO case.
> > +
> > + foreach i ${messages_for_file} {
> > + verbose "Scanning for message: $i" 4
> > +
> > + # Remove all error messages for the line [lindex $i 0]
> > + # in the source file. If we find any, success!
> > + set line [lindex $i 0]
> > + set pattern [lindex $i 2]
> > + set comment [lindex $i 3]
> > + verbose "line: $line" 4
> > + verbose "pattern: $pattern" 4
> > + verbose "comment: $comment" 4
> > + #send_user "Before:\n$text\n"
> > +
> > + # Unlike dg-test, we use $filename_without_path in this
> > pattern.
> > + # This is to ensure that we have the correct file/line
> > combination.
> > + # This imposes the restriction that the filename can't
> > contain
> > + # any regexp control characters. We have to strip the
> > path, since
> > + # e.g. the '+' in "g++.dg" wouldn't be valid.
> > + set pat
> > "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[^\n\
> > ]*\n?)+"
> > + if {[regsub -all $pat $text "\n" text]} {
> > + set text [string trimleft $text]
> > + set ok pass
> > + set uhoh fail
> > + } else {
> > + set ok fail
> > + set uhoh pass
> > + }
> > + #send_user "After:\n$text\n"
> > +
> > + # $line will either be a formatted line number or a number
> > all by
> > + # itself. Delete the formatting.
> > + scan $line ${dg-linenum-format} line
> > +
> > + # Unlike dg-test, add the filename to the PASS/FAIL message
> > (rather
> > + # than just the line number) so that the user can identify
> > the
> > + # pertinent directive.
> > + set describe_where "$filename_without_path line $line"
> > +
> > + # Issue the PASS/FAIL, adding "LTO" to the messages (e.g.
> > "LTO errors")
> > + # to distinguish them from the non-LTO case (in case we
> > ever need to
> > + # support both).
> > + switch [lindex $i 1] {
> > + "ERROR" {
> > + $ok "$name $comment (test for LTO errors,
> > $describe_where)"
> > + }
> > + "XERROR" {
> > + x$ok "$name $comment (test for LTO errors,
> > $describe_where)"
> > + }
> > + "WARNING" {
> > + $ok "$name $comment (test for LTO warnings,
> > $describe_where)"
> > + }
> > + "XWARNING" {
> > + x$ok "$name $comment (test for LTO warnings,
> > $describe_where)"
> > + }
> > + "BOGUS" {
> > + $uhoh "$name $comment (test for LTO bogus messages,
> > $describe_where)"
> > + }
> > + "XBOGUS" {
> > + x$uhoh "$name $comment (test for LTO bogus
> > messages, $describe_where)"
> > + }
> > + "BUILD" {
> > + $uhoh "$name $comment (test for LTO build failure,
> > $describe_where)"
> > + }
> > + "XBUILD" {
> > + x$uhoh "$name $comment (test for LTO build failure,
> > $describe_where)"
> > + }
> > + "EXEC" { }
> > + "XEXEC" { }
> > + }
> > + }
> > + return $text
> > +}
> > +
> > +# Support for checking for link-time diagnostics: check for
> > +# the expected diagnostics within TEXT, issuing PASS/FAIL results.
> > +# Return TEXT, stripped of any diagnostics that were handled.
> > +#
> > +# MESSAGES_BY_FILE is a dict; the keys are source files (with
> > paths)
> > +# the values are lists of expected messages, akin to DejaGnu's
> > "dg-messages"
> > +# variable.
> > +# TEXT is the textual output from the LTO link.
> > +
> > +proc lto_handle_diagnostics { messages_by_file text } {
> > + global testcase
> > +
> > + verbose "lto_handle_diagnostics: entry: $text" 2
> > + verbose " messages_by_file $messages_by_file" 3
> > +
> > + dict for {src dg-messages} $messages_by_file {
> > + set text [lto_handle_diagnostics_for_file $testcase $src \
> > + ${dg-messages} $text]
> > + }
> > +
> > + verbose "lto_handle_diagnostics: exit: $text" 2
> > +
> > + return $text
> > +}
> > +
> > # Prune messages that aren't useful.
> >
> > proc lto_prune_warns { text } {
> > @@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
> > regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]* value=\[^\n\]*;
> > file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
> > regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken" $text ""
> > text
> >
> > + # Ignore informational notes.
> > + regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
> > +
> > verbose "lto_prune_warns: exit: $text" 2
> >
> > return $text
> > @@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile
> > optstr xfaildata } {
> > # OPTALL is a list of compiler and linker options to use for all
> > tests
> > # OPTFILE is a list of compiler and linker options to use for this
> > test
> > # OPTSTR is the list of options to list in messages
> > -proc lto-link-and-maybe-run { testname objlist dest optall optfile
> > optstr } {
> > +# MESSAGES_BY_FILE is a dict of (src, dg-messages)
> > +proc lto-link-and-maybe-run { testname objlist dest optall optfile
> > optstr \
> > + messages_by_file } {
> > global testcase
> > global tool
> > global compile_type
> > global board_info
> >
> > + verbose "lto-link-and-maybe-run" 2
> > + verbose " messages_by_file $messages_by_file" 3
> > +
> > # Check that all of the objects were built successfully.
> > foreach obj [split $objlist] {
> > if ![file_on_host exists $obj] then {
> > @@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname
> > objlist dest optall optfile optstr } {
> > set board_info($target_board,ldscript) $saved_ldscript
> > }
> >
> > + # Check for diagnostics specified by directives
> > + set comp_output [lto_handle_diagnostics $messages_by_file
> > $comp_output]
> > +
> > # Prune unimportant visibility warnings before checking
> > output.
> > set comp_output [lto_prune_warns $comp_output]
> >
> > if ![${tool}_check_compile "$testcase $testname link" $optstr
> > \
> > $dest $comp_output] then {
> > - unresolved "$testcase $testname execute $optstr"
> > + if { ![string compare "execute" $compile_type] } {
> > + unresolved "$testcase $testname execute $optstr"
> > + }
> > return
> > }
> >
> > @@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname objlist
> > dest optall optfile optstr } {
> > $status "$testcase $testname execute $optstr"
> > }
> >
> > +# Potentially handle the given dg- directive (a list)
> > +# Return true is the directive was handled, false otherwise.
> > +
> > +proc lto-can-handle-directive { op } {
> > + set cmd [lindex $op 0]
> > +
> > + # dg-warning and dg-message append to dg-messages.
> > + upvar dg-messages dg-messages
> > +
> > + # A list of directives to recognize, and a list of directives
> > + # to remap them to.
> > + # For example, "dg-lto-warning" is implemented by calling "dg-
> > warning".
> > + set directives { dg-lto-warning dg-lto-message }
> > + set remapped_directives { dg-warning dg-message }
> > +
> > + set idx [lsearch -exact $directives $cmd]
> > + if { $idx != -1 } {
> > + verbose "remapping from: $op" 4
> > +
> > + set remapped_cmd [lindex $remapped_directives $idx]
> > + set op [lreplace $op 0 0 $remapped_cmd]
> > +
> > + verbose "remapped to: $op" 4
> > +
> > + set status [catch "$op" errmsg]
> > + if { $status != 0 } {
> > + if { 0 && [info exists errorInfo] } {
> > + # This also prints a backtrace which will just
> > confuse
> > + # testcase writers, so it's disabled.
> > + perror "$name: $errorInfo\n"
> > + } else {
> > + perror "$name: $errmsg for \"$op\"\n"
> > + }
> > + # ??? The call to unresolved here is necessary to clear
> > `errcnt'.
> > + # What we really need is a proc like perror that
> > doesn't set errcnt.
> > + # It should also set exit_status to 1.
> > + unresolved "$name: $errmsg for \"$op\""
> > + }
> > +
> > + return true
> > + }
> > +
> > + return false
> > +}
> > +
> > # lto-get-options-main -- get target requirements for a test and
> > # options for the primary source file and the test as a whole
> > #
> > @@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
> > upvar dg-final-code dg-final-code
> > set dg-final-code ""
> >
> > + # dg-warning and dg-message append to dg-messages.
> > + upvar dg-messages-by-file dg-messages-by-file
> > + set dg-messages ""
> > +
> > set tmp [dg-get-options $src]
> > verbose "getting options for $src: $tmp"
> > foreach op $tmp {
> > @@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
> > } else {
> > append dg-final-code "[lindex $op 2]\n"
> > }
> > - } else {
> > + } elseif { ![lto-can-handle-directive $op] } {
> > # Ignore unrecognized dg- commands, but warn about
> > them.
> > warning "lto.exp does not support $cmd"
> > }
> > }
> >
> > + verbose "dg-messages: ${dg-messages}" 3
> > + dict append dg-messages-by-file $src ${dg-messages}
> > +
> > # Return flags to use for compiling the primary source file
> > and for
> > # linking.
> > verbose "dg-extra-tool-flags for main is ${dg-extra-tool-
> > flags}"
> > @@ -373,6 +554,10 @@ proc lto-get-options { src } {
> > # dg-xfail-if needs access to dg-do-what.
> > upvar dg-do-what dg-do-what
> >
> > + # dg-warning appends to dg-messages.
> > + upvar dg-messages-by-file dg-messages-by-file
> > + set dg-messages ""
> > +
> > set tmp [dg-get-options $src]
> > foreach op $tmp {
> > set cmd [lindex $op 0]
> > @@ -386,12 +571,15 @@ proc lto-get-options { src } {
> > }
> > } elseif { [string match "dg-require-*" $cmd] } {
> > warning "lto.exp does not support $cmd in secondary
> > source files"
> > - } else {
> > + } elseif { ![lto-can-handle-directive $op] } {
> > # Ignore unrecognized dg- commands, but warn about
> > them.
> > warning "lto.exp does not support $cmd in secondary
> > source files"
> > }
> > }
> >
> > + verbose "dg-messages: ${dg-messages}" 3
> > + dict append dg-messages-by-file $src ${dg-messages}
> > +
> > return ${dg-extra-tool-flags}
> > }
> >
> > @@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
> > verbose "lto-execute: $src1" 1
> > set compile_type "run"
> > set dg-do-what [list ${dg-do-what-default} "" P]
> > + set dg-messages-by-file [dict create]
> > set extra_flags(0) [lto-get-options-main $src1]
> > set compile_xfail(0) ""
> >
> > @@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
> > lto-link-and-maybe-run \
> > "[lindex $obj_list 0]-[lindex $obj_list end]" \
> > $obj_list $execname $filtered ${dg-extra-ld-
> > options} \
> > - $filtered
> > + $filtered ${dg-messages-by-file}
> > }
> >
> >
> > --
> > 1.8.5.3
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
2018-01-05 22:55 ` David Malcolm
@ 2018-01-06 7:44 ` Richard Biener
2018-01-08 19:38 ` David Malcolm
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2018-01-06 7:44 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, Jan Hubicka
On January 5, 2018 11:55:11 PM GMT+01:00, David Malcolm <dmalcolm@redhat.com> wrote:
>On Fri, 2018-01-05 at 10:36 +0100, Richard Biener wrote:
>> On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <dmalcolm@redhat.com>
>> wrote:
>> > PR lto/83121 reports an ICE deep inside the linemap code when -Wodr
>> > reports on a type mismatch.
>> >
>> > The root cause is that the warning can access the
>> > DECL_SOURCE_LOCATION
>> > of a streamed-in decl before the lto_location_cache has been
>> > applied.
>> >
>> > lto_location_cache::input_location stores RESERVED_LOCATION_COUNT
>> > (==2)
>> > as a poison value until the cache is applied:
>> > 250 /* Keep value RESERVED_LOCATION_COUNT in *loc as linemap
>> > lookups will
>> > 251 ICE on it. */
>> >
>> > The fix is relatively simple: apply the cache before reading the
>> > DECL_SOURCE_LOCATION.
>> >
>> > (I wonder if we should instead have a INVALID_LOCATION value to
>> > handle
>> > this case more explicitly? e.g. 0xffffffff? or reserve 2 in
>> > libcpp for
>> > that purpose, and have the non-reserved locations start at
>> > 3? Either
>> > would be more invasive, though)
>> >
>> > Triggering the ICE was fiddly: it seems to be affected by many
>> > things,
>> > including the order of files, and (I think) by filenames. My
>> > theory is
>> > that it's affected by the ordering of the tree nodes in the LTO
>> > stream:
>> > for the ICE to occur, the types in question need to be compared
>> > before
>> > some other operation flushes the lto_location_cache. This ordering
>> > is affected by the hash-based ordering in DFS in lto-streamer-
>> > out.c, which
>> > might explain why r255066 seemed to trigger the bug; the only
>> > relevant
>> > change to LTO there seemed to be:
>> > * lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and
>> > DECL_PADDING_P.
>> > If so, then the bug was presumably already present, but hidden.
>> >
>> > The patch also adds regression test coverage for the ICE, which is
>> > more
>> > involved - as far as I can tell, we don't have an existing way to
>> > verify
>> > diagnostics emitted during link-time optimization.
>> >
>> > Hence the patch adds some machinery to lib/lto.exp to support two
>> > new
>> > directives: dg-lto-warning and dg-lto-message, corresponding to
>> > dg-warning and dg-message respectively, where the diagnostics are
>> > expected to be emitted at link-time.
>> >
>> > The test case includes examples of LTO warnings and notes in both
>> > the
>> > primary and secondary source files
>> >
>> > Doing so required reusing the logic from DejaGnu for handling
>> > diagnostics.
>> > Unfortunately the pertinent code is a 50 line loop within a ~200
>> > line Tcl
>> > function in dg.exp (dg-test), so I had to copy it from DejaGnu,
>> > making
>> > various changes as necessary (see lto_handle_diagnostics_for_file
>> > in the
>> > patch; for example the LTO version supports multiple source files,
>> > identifying which source file emitted a diagnostic).
>> >
>> > For non-LTO diagnostics we currently ignore surplus "note"
>> > diagnostics.
>> > This patch updates lto_prune_warns to follow this behavior (since
>> > otherwise we'd need numerous dg-lto-message directives for the
>> > motivating
>> > test case).
>> >
>> > The patch adds these PASS results to g++.sum:
>> >
>> > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
>> > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
>> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line
>> > 6)
>> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line
>> > 8)
>> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line
>> > 2)
>> > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line
>> > 3)
>> > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o
>> > link, -O0 -flto
>> >
>> > The output for dg-lto-message above refers to "warnings", rather
>> > than
>> > "messages" but that's the same as for the non-LTO case, where dg-
>> > message
>> > also refers to "warnings".
>> >
>> > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
>> >
>> > OK for trunk?
>>
>> Hmm, but we do this in warn_odr already? How's that not enough?
>>
>> At least it seems the place you add this isn't ideal (not at the
>> "root cause").
>>
>> Richard.
>
>[CCing Honza]
>
>Yes, warn_odr does apply the cache, but this warning is coming from
>warn_types_mismatch.
>
>Of the various calls to warn_types_mismatch, most are immediately after
>a warn_odr guarded by if (warn && warned) or if (warned), and if
>warn_odr writes back true to *warned, it has definitely applied the
>cache.
>
>However, the 7-argument overload of odr_types_equivalent_p can
>also call warn_types_mismatch, passing in the location_t values it
>received.
>
>The problem occurs with this call stack, when:
>
> add_type_duplicate, passes DECL_SOURCE_LOCATIONs to:
> odr_types_equivalent_p (7-argument overload) - which calls:
> warn_types_mismatch, which calls:
> warning_at with the given location_t
>
>where the DECL_SOURCE_LOCATION might not yet have been fixed up yet.
>My patch adds the fixup there.
>
>This behavior seems to have been introduced by r224248 (aka
>379ca7f842e5b13109b689b3c732741cab3d178e), Honza's fix for "PR
>lto/65378" (though that seems to have been a typo, as that bug is
>unrelated to the changes in that commit); the relevant ML post was:
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00523.html
>
>
>Would you prefer this if I reworked the two-liner into something like
>an "lto_location_cache::ensure_cache_applied" function?
No, but put it in warn_types_mismatch?
Richard.
>Thanks
>Dave
>
>>
>> > gcc/ChangeLog:
>> > PR lto/83121
>> > * ipa-devirt.c (add_type_duplicate): When comparing memory
>> > layout,
>> > call the lto_location_cache before reading the
>> > DECL_SOURCE_LOCATION of the types.
>> >
>> > gcc/testsuite/ChangeLog:
>> > PR lto/83121
>> > * g++.dg/lto/pr83121_0.C: New test case.
>> > * g++.dg/lto/pr83121_1.C: New test case.
>> > * lib/lto.exp (lto_handle_diagnostics_for_file): New
>> > procedure,
>> > adapted from DejaGnu's dg-test.
>> > (lto_handle_diagnostics): New procedure.
>> > (lto_prune_warns): Ignore informational notes.
>> > (lto-link-and-maybe-run): Add "messages_by_file" param.
>> > Call lto_handle_diagnostics. Avoid issuing "unresolved"
>> > for
>> > "execute" when "link" fails if "execute" was not specified.
>> > (lto-can-handle-directive): New procedure.
>> > (lto-get-options-main): Call lto-can-handle-directive. Add
>
>> > a
>> > dg-messages local, using it to set the caller's
>> > dg-messages-by-file for the given source file.
>> > (lto-get-options): Likewise.
>> > (lto-execute): Add dg-messages-by-file local, and pass it
>> > to
>> > lto-link-and-maybe-run.
>> > ---
>> > gcc/ipa-devirt.c | 7 +-
>> > gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
>> > gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
>> > gcc/testsuite/lib/lto.exp | 199
>> > ++++++++++++++++++++++++++++++++++-
>> > 4 files changed, 222 insertions(+), 6 deletions(-)
>> > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
>> >
>> > diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
>> > index 540f038..f3d2e4a 100644
>> > --- a/gcc/ipa-devirt.c
>> > +++ b/gcc/ipa-devirt.c
>> > @@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree type)
>> > }
>> > }
>> >
>> > - /* Next compare memory layout. */
>> > + /* Next compare memory layout.
>> > + The DECL_SOURCE_LOCATIONs in this invocation came from LTO
>> > streaming.
>> > + We must apply the location cache to ensure that they are
>> > valid
>> > + before we can pass them to odr_types_equivalent_p (PR
>> > lto/83121). */
>> > + if (lto_location_cache::current_cache)
>> > + lto_location_cache::current_cache->apply_location_cache ();
>> > if (!odr_types_equivalent_p (val->type, type,
>> > !flag_ltrans && !val->odr_violated
>> > && !warned,
>> > &warned, &visited,
>> > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > b/gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > new file mode 100644
>> > index 0000000..ef894c7
>> > --- /dev/null
>> > +++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > @@ -0,0 +1,12 @@
>> > +// { dg-lto-do link }
>> > +// { dg-lto-options {{-O0 -flto}} }
>> > +/* We need -O0 to avoid the "Environment" locals in the test
>> > functions
>> > + from being optimized away. */
>> > +
>> > +struct Environment { // { dg-lto-warning "8: type 'struct
>> > Environment' violates the C\\+\\+ One Definition Rule" }
>> > + struct AsyncHooks {
>> > + int providers_[2]; // { dg-lto-message "a field of same name
>> > but different type is defined in another translation unit" }
>> > + };
>> > + AsyncHooks async_hooks_;
>> > +};
>> > +void fn2() { Environment a; }
>> > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > b/gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > new file mode 100644
>> > index 0000000..2aef1b5
>> > --- /dev/null
>> > +++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > @@ -0,0 +1,10 @@
>> > +struct Environment {
>> > + struct AsyncHooks { // { dg-lto-warning "10: type 'struct
>> > AsyncHooks' violates the C\\+\\+ One Definition Rule" }
>> > + int providers_[1]; // { dg-lto-message "the first difference
>> > of corresponding definitions is field 'providers_'" }
>> > + };
>> > + AsyncHooks async_hooks_;
>> > +};
>> > +void fn1() { Environment a; }
>> > +int main ()
>> > +{
>> > +}
>> > diff --git a/gcc/testsuite/lib/lto.exp b/gcc/testsuite/lib/lto.exp
>> > index 477f683..e69d8d4 100644
>> > --- a/gcc/testsuite/lib/lto.exp
>> > +++ b/gcc/testsuite/lib/lto.exp
>> > @@ -16,6 +16,122 @@
>> >
>> > # Contributed by Diego Novillo <dnovillo@google.com>
>> >
>> > +# A subroutine of lto_handle_diagnostics: check TEXT for the
>> > expected
>> > +# diagnostics for one specific source file, issuing PASS/FAIL
>> > results.
>> > +# Return TEXT, stripped of any diagnostics that were handled.
>> > +#
>> > +# NAME is the testcase name to use when reporting PASS/FAIL
>> > results.
>> > +# FILENAME is the name (with full path) of the file we're
>> > interested in.
>> > +# MESSAGES_FOR_FILE is a list of expected messages, akin to
>> > DejaGnu's
>> > +# "dg-messages" variable.
>> > +# TEXT is the textual output from the LTO link.
>> > +
>> > +proc lto_handle_diagnostics_for_file { name filename
>> > messages_for_file text } {
>> > + global dg-linenum-format
>> > +
>> > + set filename_without_path [file tail $filename]
>> > +
>> > + # This loop is adapted from the related part of DejaGnu's dg-
>> > test,
>> > + # with changes as detailed below to cope with the LTO case.
>> > +
>> > + foreach i ${messages_for_file} {
>> > + verbose "Scanning for message: $i" 4
>> > +
>> > + # Remove all error messages for the line [lindex $i 0]
>> > + # in the source file. If we find any, success!
>> > + set line [lindex $i 0]
>> > + set pattern [lindex $i 2]
>> > + set comment [lindex $i 3]
>> > + verbose "line: $line" 4
>> > + verbose "pattern: $pattern" 4
>> > + verbose "comment: $comment" 4
>> > + #send_user "Before:\n$text\n"
>> > +
>> > + # Unlike dg-test, we use $filename_without_path in this
>> > pattern.
>> > + # This is to ensure that we have the correct file/line
>> > combination.
>> > + # This imposes the restriction that the filename can't
>> > contain
>> > + # any regexp control characters. We have to strip the
>> > path, since
>> > + # e.g. the '+' in "g++.dg" wouldn't be valid.
>> > + set pat
>> > "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[^\n\
>> > ]*\n?)+"
>> > + if {[regsub -all $pat $text "\n" text]} {
>> > + set text [string trimleft $text]
>> > + set ok pass
>> > + set uhoh fail
>> > + } else {
>> > + set ok fail
>> > + set uhoh pass
>> > + }
>> > + #send_user "After:\n$text\n"
>> > +
>> > + # $line will either be a formatted line number or a number
>> > all by
>> > + # itself. Delete the formatting.
>> > + scan $line ${dg-linenum-format} line
>> > +
>> > + # Unlike dg-test, add the filename to the PASS/FAIL message
>> > (rather
>> > + # than just the line number) so that the user can identify
>> > the
>> > + # pertinent directive.
>> > + set describe_where "$filename_without_path line $line"
>> > +
>> > + # Issue the PASS/FAIL, adding "LTO" to the messages (e.g.
>> > "LTO errors")
>> > + # to distinguish them from the non-LTO case (in case we
>> > ever need to
>> > + # support both).
>> > + switch [lindex $i 1] {
>> > + "ERROR" {
>> > + $ok "$name $comment (test for LTO errors,
>> > $describe_where)"
>> > + }
>> > + "XERROR" {
>> > + x$ok "$name $comment (test for LTO errors,
>> > $describe_where)"
>> > + }
>> > + "WARNING" {
>> > + $ok "$name $comment (test for LTO warnings,
>> > $describe_where)"
>> > + }
>> > + "XWARNING" {
>> > + x$ok "$name $comment (test for LTO warnings,
>> > $describe_where)"
>> > + }
>> > + "BOGUS" {
>> > + $uhoh "$name $comment (test for LTO bogus messages,
>> > $describe_where)"
>> > + }
>> > + "XBOGUS" {
>> > + x$uhoh "$name $comment (test for LTO bogus
>> > messages, $describe_where)"
>> > + }
>> > + "BUILD" {
>> > + $uhoh "$name $comment (test for LTO build failure,
>> > $describe_where)"
>> > + }
>> > + "XBUILD" {
>> > + x$uhoh "$name $comment (test for LTO build failure,
>> > $describe_where)"
>> > + }
>> > + "EXEC" { }
>> > + "XEXEC" { }
>> > + }
>> > + }
>> > + return $text
>> > +}
>> > +
>> > +# Support for checking for link-time diagnostics: check for
>> > +# the expected diagnostics within TEXT, issuing PASS/FAIL results.
>> > +# Return TEXT, stripped of any diagnostics that were handled.
>> > +#
>> > +# MESSAGES_BY_FILE is a dict; the keys are source files (with
>> > paths)
>> > +# the values are lists of expected messages, akin to DejaGnu's
>> > "dg-messages"
>> > +# variable.
>> > +# TEXT is the textual output from the LTO link.
>> > +
>> > +proc lto_handle_diagnostics { messages_by_file text } {
>> > + global testcase
>> > +
>> > + verbose "lto_handle_diagnostics: entry: $text" 2
>> > + verbose " messages_by_file $messages_by_file" 3
>> > +
>> > + dict for {src dg-messages} $messages_by_file {
>> > + set text [lto_handle_diagnostics_for_file $testcase $src \
>> > + ${dg-messages} $text]
>> > + }
>> > +
>> > + verbose "lto_handle_diagnostics: exit: $text" 2
>> > +
>> > + return $text
>> > +}
>> > +
>> > # Prune messages that aren't useful.
>> >
>> > proc lto_prune_warns { text } {
>> > @@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
>> > regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]* value=\[^\n\]*;
>> > file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
>> > regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken" $text ""
>> > text
>> >
>> > + # Ignore informational notes.
>> > + regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
>> > +
>> > verbose "lto_prune_warns: exit: $text" 2
>> >
>> > return $text
>> > @@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile
>> > optstr xfaildata } {
>> > # OPTALL is a list of compiler and linker options to use for all
>> > tests
>> > # OPTFILE is a list of compiler and linker options to use for this
>> > test
>> > # OPTSTR is the list of options to list in messages
>> > -proc lto-link-and-maybe-run { testname objlist dest optall optfile
>> > optstr } {
>> > +# MESSAGES_BY_FILE is a dict of (src, dg-messages)
>> > +proc lto-link-and-maybe-run { testname objlist dest optall optfile
>> > optstr \
>> > + messages_by_file } {
>> > global testcase
>> > global tool
>> > global compile_type
>> > global board_info
>> >
>> > + verbose "lto-link-and-maybe-run" 2
>> > + verbose " messages_by_file $messages_by_file" 3
>> > +
>> > # Check that all of the objects were built successfully.
>> > foreach obj [split $objlist] {
>> > if ![file_on_host exists $obj] then {
>> > @@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname
>> > objlist dest optall optfile optstr } {
>> > set board_info($target_board,ldscript) $saved_ldscript
>> > }
>> >
>> > + # Check for diagnostics specified by directives
>> > + set comp_output [lto_handle_diagnostics $messages_by_file
>> > $comp_output]
>> > +
>> > # Prune unimportant visibility warnings before checking
>> > output.
>> > set comp_output [lto_prune_warns $comp_output]
>> >
>> > if ![${tool}_check_compile "$testcase $testname link" $optstr
>> > \
>> > $dest $comp_output] then {
>> > - unresolved "$testcase $testname execute $optstr"
>> > + if { ![string compare "execute" $compile_type] } {
>> > + unresolved "$testcase $testname execute $optstr"
>> > + }
>> > return
>> > }
>> >
>> > @@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname objlist
>> > dest optall optfile optstr } {
>> > $status "$testcase $testname execute $optstr"
>> > }
>> >
>> > +# Potentially handle the given dg- directive (a list)
>> > +# Return true is the directive was handled, false otherwise.
>> > +
>> > +proc lto-can-handle-directive { op } {
>> > + set cmd [lindex $op 0]
>> > +
>> > + # dg-warning and dg-message append to dg-messages.
>> > + upvar dg-messages dg-messages
>> > +
>> > + # A list of directives to recognize, and a list of directives
>> > + # to remap them to.
>> > + # For example, "dg-lto-warning" is implemented by calling "dg-
>> > warning".
>> > + set directives { dg-lto-warning dg-lto-message }
>> > + set remapped_directives { dg-warning dg-message }
>> > +
>> > + set idx [lsearch -exact $directives $cmd]
>> > + if { $idx != -1 } {
>> > + verbose "remapping from: $op" 4
>> > +
>> > + set remapped_cmd [lindex $remapped_directives $idx]
>> > + set op [lreplace $op 0 0 $remapped_cmd]
>> > +
>> > + verbose "remapped to: $op" 4
>> > +
>> > + set status [catch "$op" errmsg]
>> > + if { $status != 0 } {
>> > + if { 0 && [info exists errorInfo] } {
>> > + # This also prints a backtrace which will just
>> > confuse
>> > + # testcase writers, so it's disabled.
>> > + perror "$name: $errorInfo\n"
>> > + } else {
>> > + perror "$name: $errmsg for \"$op\"\n"
>> > + }
>> > + # ??? The call to unresolved here is necessary to clear
>> > `errcnt'.
>> > + # What we really need is a proc like perror that
>> > doesn't set errcnt.
>> > + # It should also set exit_status to 1.
>> > + unresolved "$name: $errmsg for \"$op\""
>> > + }
>> > +
>> > + return true
>> > + }
>> > +
>> > + return false
>> > +}
>> > +
>> > # lto-get-options-main -- get target requirements for a test and
>> > # options for the primary source file and the test as a whole
>> > #
>> > @@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
>> > upvar dg-final-code dg-final-code
>> > set dg-final-code ""
>> >
>> > + # dg-warning and dg-message append to dg-messages.
>> > + upvar dg-messages-by-file dg-messages-by-file
>> > + set dg-messages ""
>> > +
>> > set tmp [dg-get-options $src]
>> > verbose "getting options for $src: $tmp"
>> > foreach op $tmp {
>> > @@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
>> > } else {
>> > append dg-final-code "[lindex $op 2]\n"
>> > }
>> > - } else {
>> > + } elseif { ![lto-can-handle-directive $op] } {
>> > # Ignore unrecognized dg- commands, but warn about
>> > them.
>> > warning "lto.exp does not support $cmd"
>> > }
>> > }
>> >
>> > + verbose "dg-messages: ${dg-messages}" 3
>> > + dict append dg-messages-by-file $src ${dg-messages}
>> > +
>> > # Return flags to use for compiling the primary source file
>> > and for
>> > # linking.
>> > verbose "dg-extra-tool-flags for main is ${dg-extra-tool-
>> > flags}"
>> > @@ -373,6 +554,10 @@ proc lto-get-options { src } {
>> > # dg-xfail-if needs access to dg-do-what.
>> > upvar dg-do-what dg-do-what
>> >
>> > + # dg-warning appends to dg-messages.
>> > + upvar dg-messages-by-file dg-messages-by-file
>> > + set dg-messages ""
>> > +
>> > set tmp [dg-get-options $src]
>> > foreach op $tmp {
>> > set cmd [lindex $op 0]
>> > @@ -386,12 +571,15 @@ proc lto-get-options { src } {
>> > }
>> > } elseif { [string match "dg-require-*" $cmd] } {
>> > warning "lto.exp does not support $cmd in secondary
>> > source files"
>> > - } else {
>> > + } elseif { ![lto-can-handle-directive $op] } {
>> > # Ignore unrecognized dg- commands, but warn about
>> > them.
>> > warning "lto.exp does not support $cmd in secondary
>> > source files"
>> > }
>> > }
>> >
>> > + verbose "dg-messages: ${dg-messages}" 3
>> > + dict append dg-messages-by-file $src ${dg-messages}
>> > +
>> > return ${dg-extra-tool-flags}
>> > }
>> >
>> > @@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
>> > verbose "lto-execute: $src1" 1
>> > set compile_type "run"
>> > set dg-do-what [list ${dg-do-what-default} "" P]
>> > + set dg-messages-by-file [dict create]
>> > set extra_flags(0) [lto-get-options-main $src1]
>> > set compile_xfail(0) ""
>> >
>> > @@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
>> > lto-link-and-maybe-run \
>> > "[lindex $obj_list 0]-[lindex $obj_list end]" \
>> > $obj_list $execname $filtered ${dg-extra-ld-
>> > options} \
>> > - $filtered
>> > + $filtered ${dg-messages-by-file}
>> > }
>> >
>> >
>> > --
>> > 1.8.5.3
>> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
2018-01-06 7:44 ` Richard Biener
@ 2018-01-08 19:38 ` David Malcolm
2018-01-15 10:09 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2018-01-08 19:38 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, Jan Hubicka
On Sat, 2018-01-06 at 08:44 +0100, Richard Biener wrote:
> On January 5, 2018 11:55:11 PM GMT+01:00, David Malcolm <dmalcolm@red
> hat.com> wrote:
> > On Fri, 2018-01-05 at 10:36 +0100, Richard Biener wrote:
> > > On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <dmalcolm@redhat.c
> > > om>
> > > wrote:
> > > > PR lto/83121 reports an ICE deep inside the linemap code when
> > > > -Wodr
> > > > reports on a type mismatch.
> > > >
> > > > The root cause is that the warning can access the
> > > > DECL_SOURCE_LOCATION
> > > > of a streamed-in decl before the lto_location_cache has been
> > > > applied.
> > > >
> > > > lto_location_cache::input_location stores
> > > > RESERVED_LOCATION_COUNT
> > > > (==2)
> > > > as a poison value until the cache is applied:
> > > > 250 /* Keep value RESERVED_LOCATION_COUNT in *loc as
> > > > linemap
> > > > lookups will
> > > > 251 ICE on it. */
> > > >
> > > > The fix is relatively simple: apply the cache before reading
> > > > the
> > > > DECL_SOURCE_LOCATION.
> > > >
> > > > (I wonder if we should instead have a INVALID_LOCATION value to
> > > > handle
> > > > this case more explicitly? e.g. 0xffffffff? or reserve 2 in
> > > > libcpp for
> > > > that purpose, and have the non-reserved locations start at
> > > > 3? Either
> > > > would be more invasive, though)
> > > >
> > > > Triggering the ICE was fiddly: it seems to be affected by many
> > > > things,
> > > > including the order of files, and (I think) by filenames. My
> > > > theory is
> > > > that it's affected by the ordering of the tree nodes in the LTO
> > > > stream:
> > > > for the ICE to occur, the types in question need to be compared
> > > > before
> > > > some other operation flushes the lto_location_cache. This
> > > > ordering
> > > > is affected by the hash-based ordering in DFS in lto-streamer-
> > > > out.c, which
> > > > might explain why r255066 seemed to trigger the bug; the only
> > > > relevant
> > > > change to LTO there seemed to be:
> > > > * lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and
> > > > DECL_PADDING_P.
> > > > If so, then the bug was presumably already present, but hidden.
> > > >
> > > > The patch also adds regression test coverage for the ICE, which
> > > > is
> > > > more
> > > > involved - as far as I can tell, we don't have an existing way
> > > > to
> > > > verify
> > > > diagnostics emitted during link-time optimization.
> > > >
> > > > Hence the patch adds some machinery to lib/lto.exp to support
> > > > two
> > > > new
> > > > directives: dg-lto-warning and dg-lto-message, corresponding to
> > > > dg-warning and dg-message respectively, where the diagnostics
> > > > are
> > > > expected to be emitted at link-time.
> > > >
> > > > The test case includes examples of LTO warnings and notes in
> > > > both
> > > > the
> > > > primary and secondary source files
> > > >
> > > > Doing so required reusing the logic from DejaGnu for handling
> > > > diagnostics.
> > > > Unfortunately the pertinent code is a 50 line loop within a
> > > > ~200
> > > > line Tcl
> > > > function in dg.exp (dg-test), so I had to copy it from DejaGnu,
> > > > making
> > > > various changes as necessary (see
> > > > lto_handle_diagnostics_for_file
> > > > in the
> > > > patch; for example the LTO version supports multiple source
> > > > files,
> > > > identifying which source file emitted a diagnostic).
> > > >
> > > > For non-LTO diagnostics we currently ignore surplus "note"
> > > > diagnostics.
> > > > This patch updates lto_prune_warns to follow this behavior
> > > > (since
> > > > otherwise we'd need numerous dg-lto-message directives for the
> > > > motivating
> > > > test case).
> > > >
> > > > The patch adds these PASS results to g++.sum:
> > > >
> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C
> > > > line
> > > > 6)
> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C
> > > > line
> > > > 8)
> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C
> > > > line
> > > > 2)
> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C
> > > > line
> > > > 3)
> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o
> > > > link, -O0 -flto
> > > >
> > > > The output for dg-lto-message above refers to "warnings",
> > > > rather
> > > > than
> > > > "messages" but that's the same as for the non-LTO case, where
> > > > dg-
> > > > message
> > > > also refers to "warnings".
> > > >
> > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
> > > >
> > > > OK for trunk?
> > >
> > > Hmm, but we do this in warn_odr already? How's that not enough?
> > >
> > > At least it seems the place you add this isn't ideal (not at the
> > > "root cause").
> > >
> > > Richard.
> >
> > [CCing Honza]
> >
> > Yes, warn_odr does apply the cache, but this warning is coming from
> > warn_types_mismatch.
> >
> > Of the various calls to warn_types_mismatch, most are immediately
> > after
> > a warn_odr guarded by if (warn && warned) or if (warned), and if
> > warn_odr writes back true to *warned, it has definitely applied the
> > cache.
> >
> > However, the 7-argument overload of odr_types_equivalent_p can
> > also call warn_types_mismatch, passing in the location_t values it
> > received.
> >
> > The problem occurs with this call stack, when:
> >
> > add_type_duplicate, passes DECL_SOURCE_LOCATIONs to:
> > odr_types_equivalent_p (7-argument overload) - which calls:
> > warn_types_mismatch, which calls:
> > warning_at with the given location_t
> >
> > where the DECL_SOURCE_LOCATION might not yet have been fixed up
> > yet.
> > My patch adds the fixup there.
> >
> > This behavior seems to have been introduced by r224248 (aka
> > 379ca7f842e5b13109b689b3c732741cab3d178e), Honza's fix for "PR
> > lto/65378" (though that seems to have been a typo, as that bug is
> > unrelated to the changes in that commit); the relevant ML post was:
> > https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00523.html
> >
> >
> > Would you prefer this if I reworked the two-liner into something
> > like
> > an "lto_location_cache::ensure_cache_applied" function?
>
> No, but put it in warn_types_mismatch?
Hmm... putting it in warn_types_mismatch dosn't fix it, for the same
reason as in the earlier description (or maybe I'm misunderstanding
you?).
As I understand it, the lto_location_cache effectively contains a vec
of location_t * and expanded locations, and writes back to the
location_t * once those "expanded" locations can be turned back into
valid location_t values.
The issue is that add_type_duplicate takes the DECL_SOURCE_LOCATION of
a decl before the cache is applied, and that location_t gets passed
around as a parameter. At that point, even if the cache is applied and
the DECL_SOURCE_LOCATION is fixed, the local location_t values being
passed around aren't going to be touched (they're not in the lto
location cache - they're on the stack or in registers).
FWIW, the backtrace at the point of failure looks like as follows; the
invalid loc=2 values can be seen there. Note how the invalid
location_t value of 2 is read in frame 17, and is passed around as a
param in frames 16 down to 10, leading to the crash:
(gdb) bt
#0 fancy_abort (file=0x2135f00 "../../src/libcpp/line-map.c", line=990,
function=0x2136390 <linemap_ordinary_map_lookup(line_maps*, unsigned int)::__FUNCTION__> "linemap_ordinary_map_lookup") at ../../src/gcc/diagnostic.c:1500
#1 0x0000000001c262c7 in linemap_ordinary_map_lookup (line=2, set=<optimized out>)
at ../../src/libcpp/line-map.c:990
#2 linemap_lookup (set=set@entry=0x7ffff7ffb000, line=<optimized out>, line@entry=2)
at ../../src/libcpp/line-map.c:943
#3 0x0000000001c27294 in linemap_macro_loc_to_def_point (original_map=0x7fffffffd2e0, location=2,
set=0x7ffff7ffb000) at ../../src/libcpp/line-map.c:1448
#4 linemap_resolve_location (set=0x7ffff7ffb000, loc=<optimized out>, lrk=<optimized out>, map=0x7fffffffd2e0)
at ../../src/libcpp/line-map.c:1580
#5 0x0000000001bdc4f2 in diagnostic_report_current_module (context=0x288bce0 <global_diagnostic_context>, where=2)
at ../../src/gcc/diagnostic.c:581
#6 0x0000000000f36ae0 in diagnostic_report_current_function (context=0x288bce0 <global_diagnostic_context>,
diagnostic=0x7fffffffd3f0) at ../../src/gcc/tree-diagnostic.c:39
#7 0x0000000000f36b44 in default_tree_diagnostic_starter (context=0x288bce0 <global_diagnostic_context>,
diagnostic=0x7fffffffd3f0) at ../../src/gcc/tree-diagnostic.c:48
#8 0x0000000001bdd3b4 in diagnostic_report_diagnostic (context=0x288bce0 <global_diagnostic_context>,
diagnostic=0x7fffffffd3f0) at ../../src/gcc/diagnostic.c:985
#9 0x0000000001bdd90e in diagnostic_impl (richloc=0x7fffffffd450, opt=-1,
gmsgid=0x1cdcd40 "array types have different bounds", ap=0x7fffffffd4d8, kind=DK_NOTE)
at ../../src/gcc/diagnostic.c:1108
#10 0x0000000001bddbfa in inform (location=2, gmsgid=0x1cdcd40 "array types have different bounds")
at ../../src/gcc/diagnostic.c:1160
#11 0x0000000000bb2dd0 in warn_types_mismatch (t1=<array_type 0x7ffff1aa6dc8>, t2=<array_type 0x7ffff1aa69d8>,
loc1=2, loc2=4448) at ../../src/gcc/ipa-devirt.c:1176
#12 0x0000000000bb5245 in odr_types_equivalent_p (t1=<record_type 0x7ffff1aa6e70 AsyncHooks>,
t2=<record_type 0x7ffff1aa6b28 AsyncHooks>, warn=true, warned=0x7fffffffd7e3, visited=0x7fffffffd7b0, loc1=2,
loc2=4448) at ../../src/gcc/ipa-devirt.c:1569
#13 0x0000000000bb6ebb in add_type_duplicate (val=0x7ffff18ab570, type=<record_type 0x7ffff1aa6b28 AsyncHooks>)
at ../../src/gcc/ipa-devirt.c:1848
#14 0x0000000000bb77f2 in get_odr_type (type=<record_type 0x7ffff1aa6b28 AsyncHooks>, insert=true)
at ../../src/gcc/ipa-devirt.c:2028
#15 0x0000000000bb0ae0 in odr_subtypes_equivalent_p (t1=<record_type 0x7ffff1aa6b28 AsyncHooks>,
t2=<record_type 0x7ffff1aa6e70 AsyncHooks>, visited=0x7fffffffda10, loc1=288, loc2=2)
at ../../src/gcc/ipa-devirt.c:689
#16 0x0000000000bb5151 in odr_types_equivalent_p (t1=<record_type 0x7ffff1aa6a80 Environment>,
t2=<record_type 0x7ffff1aa6f18 Environment>, warn=true, warned=0x7fffffffda43, visited=0x7fffffffda10, loc1=288,
loc2=2) at ../../src/gcc/ipa-devirt.c:1556
#17 0x0000000000bb6ebb in add_type_duplicate (val=0x7ffff18ab4e0, type=<record_type 0x7ffff1aa6f18 Environment>)
at ../../src/gcc/ipa-devirt.c:1848
#18 0x0000000000bb77f2 in get_odr_type (type=<record_type 0x7ffff1aa6f18 Environment>, insert=true)
at ../../src/gcc/ipa-devirt.c:2028
#19 0x0000000000bb7e42 in register_odr_type (type=<record_type 0x7ffff1aa6f18 Environment>)
at ../../src/gcc/ipa-devirt.c:2111
#20 0x00000000007eae64 in lto_read_decls (decl_data=0x7ffff1696000, data=0x296c040, resolutions=...)
at ../../src/gcc/lto/lto.c:1755
#21 0x00000000007eb9c3 in lto_file_finalize (file_data=0x7ffff1696000, file=0x2968cc0)
at ../../src/gcc/lto/lto.c:2055
#22 0x00000000007eba1b in lto_create_files_from_ids (file=0x2968cc0, file_data=0x7ffff1696000, count=0x7fffffffdd4c)
at ../../src/gcc/lto/lto.c:2065
#23 0x00000000007ebb4f in lto_file_read (file=0x2968cc0, resolution_file=0x290aad0, count=0x7fffffffdd4c)
at ../../src/gcc/lto/lto.c:2106
#24 0x00000000007ef930 in read_cgraph_and_symbols (nfiles=2, fnames=0x28e5c00) at ../../src/gcc/lto/lto.c:2818
#25 0x00000000007f0c16 in lto_main () at ../../src/gcc/lto/lto.c:3323
#26 0x0000000000eb3ae6 in compile_file () at ../../src/gcc/toplev.c:455
#27 0x0000000000eb6de6 in do_compile () at ../../src/gcc/toplev.c:2076
#28 0x0000000000eb715c in toplev::main (this=0x7fffffffdeae, argc=16, argv=0x28a0aa0) at ../../src/gcc/toplev.c:2211
#29 0x0000000001bbd183 in main (argc=15, argv=0x7fffffffdfa8) at ../../src/gcc/main.c:39
My patch put the apply_location_cache immediately before the read in
add_type_duplicate, but maybe there's a better place?
Thanks
Dave
>
> Richard.
>
> > Thanks
> > Dave
> >
> > >
> > > > gcc/ChangeLog:
> > > > PR lto/83121
> > > > * ipa-devirt.c (add_type_duplicate): When comparing
> > > > memory
> > > > layout,
> > > > call the lto_location_cache before reading the
> > > > DECL_SOURCE_LOCATION of the types.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > > PR lto/83121
> > > > * g++.dg/lto/pr83121_0.C: New test case.
> > > > * g++.dg/lto/pr83121_1.C: New test case.
> > > > * lib/lto.exp (lto_handle_diagnostics_for_file): New
> > > > procedure,
> > > > adapted from DejaGnu's dg-test.
> > > > (lto_handle_diagnostics): New procedure.
> > > > (lto_prune_warns): Ignore informational notes.
> > > > (lto-link-and-maybe-run): Add "messages_by_file" param.
> > > > Call lto_handle_diagnostics. Avoid issuing
> > > > "unresolved"
> > > > for
> > > > "execute" when "link" fails if "execute" was not
> > > > specified.
> > > > (lto-can-handle-directive): New procedure.
> > > > (lto-get-options-main): Call lto-can-handle-
> > > > directive. Add
> > > > a
> > > > dg-messages local, using it to set the caller's
> > > > dg-messages-by-file for the given source file.
> > > > (lto-get-options): Likewise.
> > > > (lto-execute): Add dg-messages-by-file local, and pass
> > > > it
> > > > to
> > > > lto-link-and-maybe-run.
> > > > ---
> > > > gcc/ipa-devirt.c | 7 +-
> > > > gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
> > > > gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
> > > > gcc/testsuite/lib/lto.exp | 199
> > > > ++++++++++++++++++++++++++++++++++-
> > > > 4 files changed, 222 insertions(+), 6 deletions(-)
> > > > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
> > > > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
> > > >
> > > > diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> > > > index 540f038..f3d2e4a 100644
> > > > --- a/gcc/ipa-devirt.c
> > > > +++ b/gcc/ipa-devirt.c
> > > > @@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree
> > > > type)
> > > > }
> > > > }
> > > >
> > > > - /* Next compare memory layout. */
> > > > + /* Next compare memory layout.
> > > > + The DECL_SOURCE_LOCATIONs in this invocation came from
> > > > LTO
> > > > streaming.
> > > > + We must apply the location cache to ensure that they are
> > > > valid
> > > > + before we can pass them to odr_types_equivalent_p (PR
> > > > lto/83121). */
> > > > + if (lto_location_cache::current_cache)
> > > > + lto_location_cache::current_cache->apply_location_cache
> > > > ();
> > > > if (!odr_types_equivalent_p (val->type, type,
> > > > !flag_ltrans && !val-
> > > > >odr_violated
> > > > && !warned,
> > > > &warned, &visited,
> > > > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C
> > > > b/gcc/testsuite/g++.dg/lto/pr83121_0.C
> > > > new file mode 100644
> > > > index 0000000..ef894c7
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
> > > > @@ -0,0 +1,12 @@
> > > > +// { dg-lto-do link }
> > > > +// { dg-lto-options {{-O0 -flto}} }
> > > > +/* We need -O0 to avoid the "Environment" locals in the test
> > > > functions
> > > > + from being optimized away. */
> > > > +
> > > > +struct Environment { // { dg-lto-warning "8: type 'struct
> > > > Environment' violates the C\\+\\+ One Definition Rule" }
> > > > + struct AsyncHooks {
> > > > + int providers_[2]; // { dg-lto-message "a field of same
> > > > name
> > > > but different type is defined in another translation unit" }
> > > > + };
> > > > + AsyncHooks async_hooks_;
> > > > +};
> > > > +void fn2() { Environment a; }
> > > > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C
> > > > b/gcc/testsuite/g++.dg/lto/pr83121_1.C
> > > > new file mode 100644
> > > > index 0000000..2aef1b5
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
> > > > @@ -0,0 +1,10 @@
> > > > +struct Environment {
> > > > + struct AsyncHooks { // { dg-lto-warning "10: type 'struct
> > > > AsyncHooks' violates the C\\+\\+ One Definition Rule" }
> > > > + int providers_[1]; // { dg-lto-message "the first
> > > > difference
> > > > of corresponding definitions is field 'providers_'" }
> > > > + };
> > > > + AsyncHooks async_hooks_;
> > > > +};
> > > > +void fn1() { Environment a; }
> > > > +int main ()
> > > > +{
> > > > +}
> > > > diff --git a/gcc/testsuite/lib/lto.exp
> > > > b/gcc/testsuite/lib/lto.exp
> > > > index 477f683..e69d8d4 100644
> > > > --- a/gcc/testsuite/lib/lto.exp
> > > > +++ b/gcc/testsuite/lib/lto.exp
> > > > @@ -16,6 +16,122 @@
> > > >
> > > > # Contributed by Diego Novillo <dnovillo@google.com>
> > > >
> > > > +# A subroutine of lto_handle_diagnostics: check TEXT for the
> > > > expected
> > > > +# diagnostics for one specific source file, issuing PASS/FAIL
> > > > results.
> > > > +# Return TEXT, stripped of any diagnostics that were handled.
> > > > +#
> > > > +# NAME is the testcase name to use when reporting PASS/FAIL
> > > > results.
> > > > +# FILENAME is the name (with full path) of the file we're
> > > > interested in.
> > > > +# MESSAGES_FOR_FILE is a list of expected messages, akin to
> > > > DejaGnu's
> > > > +# "dg-messages" variable.
> > > > +# TEXT is the textual output from the LTO link.
> > > > +
> > > > +proc lto_handle_diagnostics_for_file { name filename
> > > > messages_for_file text } {
> > > > + global dg-linenum-format
> > > > +
> > > > + set filename_without_path [file tail $filename]
> > > > +
> > > > + # This loop is adapted from the related part of DejaGnu's
> > > > dg-
> > > > test,
> > > > + # with changes as detailed below to cope with the LTO
> > > > case.
> > > > +
> > > > + foreach i ${messages_for_file} {
> > > > + verbose "Scanning for message: $i" 4
> > > > +
> > > > + # Remove all error messages for the line [lindex $i 0]
> > > > + # in the source file. If we find any, success!
> > > > + set line [lindex $i 0]
> > > > + set pattern [lindex $i 2]
> > > > + set comment [lindex $i 3]
> > > > + verbose "line: $line" 4
> > > > + verbose "pattern: $pattern" 4
> > > > + verbose "comment: $comment" 4
> > > > + #send_user "Before:\n$text\n"
> > > > +
> > > > + # Unlike dg-test, we use $filename_without_path in this
> > > > pattern.
> > > > + # This is to ensure that we have the correct file/line
> > > > combination.
> > > > + # This imposes the restriction that the filename can't
> > > > contain
> > > > + # any regexp control characters. We have to strip the
> > > > path, since
> > > > + # e.g. the '+' in "g++.dg" wouldn't be valid.
> > > > + set pat
> > > > "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[
> > > > ^\n\
> > > > ]*\n?)+"
> > > > + if {[regsub -all $pat $text "\n" text]} {
> > > > + set text [string trimleft $text]
> > > > + set ok pass
> > > > + set uhoh fail
> > > > + } else {
> > > > + set ok fail
> > > > + set uhoh pass
> > > > + }
> > > > + #send_user "After:\n$text\n"
> > > > +
> > > > + # $line will either be a formatted line number or a
> > > > number
> > > > all by
> > > > + # itself. Delete the formatting.
> > > > + scan $line ${dg-linenum-format} line
> > > > +
> > > > + # Unlike dg-test, add the filename to the PASS/FAIL
> > > > message
> > > > (rather
> > > > + # than just the line number) so that the user can
> > > > identify
> > > > the
> > > > + # pertinent directive.
> > > > + set describe_where "$filename_without_path line $line"
> > > > +
> > > > + # Issue the PASS/FAIL, adding "LTO" to the messages
> > > > (e.g.
> > > > "LTO errors")
> > > > + # to distinguish them from the non-LTO case (in case we
> > > > ever need to
> > > > + # support both).
> > > > + switch [lindex $i 1] {
> > > > + "ERROR" {
> > > > + $ok "$name $comment (test for LTO errors,
> > > > $describe_where)"
> > > > + }
> > > > + "XERROR" {
> > > > + x$ok "$name $comment (test for LTO errors,
> > > > $describe_where)"
> > > > + }
> > > > + "WARNING" {
> > > > + $ok "$name $comment (test for LTO warnings,
> > > > $describe_where)"
> > > > + }
> > > > + "XWARNING" {
> > > > + x$ok "$name $comment (test for LTO warnings,
> > > > $describe_where)"
> > > > + }
> > > > + "BOGUS" {
> > > > + $uhoh "$name $comment (test for LTO bogus
> > > > messages,
> > > > $describe_where)"
> > > > + }
> > > > + "XBOGUS" {
> > > > + x$uhoh "$name $comment (test for LTO bogus
> > > > messages, $describe_where)"
> > > > + }
> > > > + "BUILD" {
> > > > + $uhoh "$name $comment (test for LTO build
> > > > failure,
> > > > $describe_where)"
> > > > + }
> > > > + "XBUILD" {
> > > > + x$uhoh "$name $comment (test for LTO build
> > > > failure,
> > > > $describe_where)"
> > > > + }
> > > > + "EXEC" { }
> > > > + "XEXEC" { }
> > > > + }
> > > > + }
> > > > + return $text
> > > > +}
> > > > +
> > > > +# Support for checking for link-time diagnostics: check for
> > > > +# the expected diagnostics within TEXT, issuing PASS/FAIL
> > > > results.
> > > > +# Return TEXT, stripped of any diagnostics that were handled.
> > > > +#
> > > > +# MESSAGES_BY_FILE is a dict; the keys are source files (with
> > > > paths)
> > > > +# the values are lists of expected messages, akin to DejaGnu's
> > > > "dg-messages"
> > > > +# variable.
> > > > +# TEXT is the textual output from the LTO link.
> > > > +
> > > > +proc lto_handle_diagnostics { messages_by_file text } {
> > > > + global testcase
> > > > +
> > > > + verbose "lto_handle_diagnostics: entry: $text" 2
> > > > + verbose " messages_by_file $messages_by_file" 3
> > > > +
> > > > + dict for {src dg-messages} $messages_by_file {
> > > > + set text [lto_handle_diagnostics_for_file $testcase
> > > > $src \
> > > > + ${dg-messages} $text]
> > > > + }
> > > > +
> > > > + verbose "lto_handle_diagnostics: exit: $text" 2
> > > > +
> > > > + return $text
> > > > +}
> > > > +
> > > > # Prune messages that aren't useful.
> > > >
> > > > proc lto_prune_warns { text } {
> > > > @@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
> > > > regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]*
> > > > value=\[^\n\]*;
> > > > file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
> > > > regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken"
> > > > $text ""
> > > > text
> > > >
> > > > + # Ignore informational notes.
> > > > + regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
> > > > +
> > > > verbose "lto_prune_warns: exit: $text" 2
> > > >
> > > > return $text
> > > > @@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile
> > > > optstr xfaildata } {
> > > > # OPTALL is a list of compiler and linker options to use for
> > > > all
> > > > tests
> > > > # OPTFILE is a list of compiler and linker options to use for
> > > > this
> > > > test
> > > > # OPTSTR is the list of options to list in messages
> > > > -proc lto-link-and-maybe-run { testname objlist dest optall
> > > > optfile
> > > > optstr } {
> > > > +# MESSAGES_BY_FILE is a dict of (src, dg-messages)
> > > > +proc lto-link-and-maybe-run { testname objlist dest optall
> > > > optfile
> > > > optstr \
> > > > + messages_by_file } {
> > > > global testcase
> > > > global tool
> > > > global compile_type
> > > > global board_info
> > > >
> > > > + verbose "lto-link-and-maybe-run" 2
> > > > + verbose " messages_by_file $messages_by_file" 3
> > > > +
> > > > # Check that all of the objects were built successfully.
> > > > foreach obj [split $objlist] {
> > > > if ![file_on_host exists $obj] then {
> > > > @@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname
> > > > objlist dest optall optfile optstr } {
> > > > set board_info($target_board,ldscript) $saved_ldscript
> > > > }
> > > >
> > > > + # Check for diagnostics specified by directives
> > > > + set comp_output [lto_handle_diagnostics $messages_by_file
> > > > $comp_output]
> > > > +
> > > > # Prune unimportant visibility warnings before checking
> > > > output.
> > > > set comp_output [lto_prune_warns $comp_output]
> > > >
> > > > if ![${tool}_check_compile "$testcase $testname link"
> > > > $optstr
> > > > \
> > > > $dest $comp_output] then {
> > > > - unresolved "$testcase $testname execute $optstr"
> > > > + if { ![string compare "execute" $compile_type] } {
> > > > + unresolved "$testcase $testname execute $optstr"
> > > > + }
> > > > return
> > > > }
> > > >
> > > > @@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname
> > > > objlist
> > > > dest optall optfile optstr } {
> > > > $status "$testcase $testname execute $optstr"
> > > > }
> > > >
> > > > +# Potentially handle the given dg- directive (a list)
> > > > +# Return true is the directive was handled, false otherwise.
> > > > +
> > > > +proc lto-can-handle-directive { op } {
> > > > + set cmd [lindex $op 0]
> > > > +
> > > > + # dg-warning and dg-message append to dg-messages.
> > > > + upvar dg-messages dg-messages
> > > > +
> > > > + # A list of directives to recognize, and a list of
> > > > directives
> > > > + # to remap them to.
> > > > + # For example, "dg-lto-warning" is implemented by calling
> > > > "dg-
> > > > warning".
> > > > + set directives { dg-lto-warning dg-lto-message }
> > > > + set remapped_directives { dg-warning dg-message }
> > > > +
> > > > + set idx [lsearch -exact $directives $cmd]
> > > > + if { $idx != -1 } {
> > > > + verbose "remapping from: $op" 4
> > > > +
> > > > + set remapped_cmd [lindex $remapped_directives $idx]
> > > > + set op [lreplace $op 0 0 $remapped_cmd]
> > > > +
> > > > + verbose "remapped to: $op" 4
> > > > +
> > > > + set status [catch "$op" errmsg]
> > > > + if { $status != 0 } {
> > > > + if { 0 && [info exists errorInfo] } {
> > > > + # This also prints a backtrace which will just
> > > > confuse
> > > > + # testcase writers, so it's disabled.
> > > > + perror "$name: $errorInfo\n"
> > > > + } else {
> > > > + perror "$name: $errmsg for \"$op\"\n"
> > > > + }
> > > > + # ??? The call to unresolved here is necessary to
> > > > clear
> > > > `errcnt'.
> > > > + # What we really need is a proc like perror that
> > > > doesn't set errcnt.
> > > > + # It should also set exit_status to 1.
> > > > + unresolved "$name: $errmsg for \"$op\""
> > > > + }
> > > > +
> > > > + return true
> > > > + }
> > > > +
> > > > + return false
> > > > +}
> > > > +
> > > > # lto-get-options-main -- get target requirements for a test
> > > > and
> > > > # options for the primary source file and the test as a whole
> > > > #
> > > > @@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
> > > > upvar dg-final-code dg-final-code
> > > > set dg-final-code ""
> > > >
> > > > + # dg-warning and dg-message append to dg-messages.
> > > > + upvar dg-messages-by-file dg-messages-by-file
> > > > + set dg-messages ""
> > > > +
> > > > set tmp [dg-get-options $src]
> > > > verbose "getting options for $src: $tmp"
> > > > foreach op $tmp {
> > > > @@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
> > > > } else {
> > > > append dg-final-code "[lindex $op 2]\n"
> > > > }
> > > > - } else {
> > > > + } elseif { ![lto-can-handle-directive $op] } {
> > > > # Ignore unrecognized dg- commands, but warn about
> > > > them.
> > > > warning "lto.exp does not support $cmd"
> > > > }
> > > > }
> > > >
> > > > + verbose "dg-messages: ${dg-messages}" 3
> > > > + dict append dg-messages-by-file $src ${dg-messages}
> > > > +
> > > > # Return flags to use for compiling the primary source
> > > > file
> > > > and for
> > > > # linking.
> > > > verbose "dg-extra-tool-flags for main is ${dg-extra-tool-
> > > > flags}"
> > > > @@ -373,6 +554,10 @@ proc lto-get-options { src } {
> > > > # dg-xfail-if needs access to dg-do-what.
> > > > upvar dg-do-what dg-do-what
> > > >
> > > > + # dg-warning appends to dg-messages.
> > > > + upvar dg-messages-by-file dg-messages-by-file
> > > > + set dg-messages ""
> > > > +
> > > > set tmp [dg-get-options $src]
> > > > foreach op $tmp {
> > > > set cmd [lindex $op 0]
> > > > @@ -386,12 +571,15 @@ proc lto-get-options { src } {
> > > > }
> > > > } elseif { [string match "dg-require-*" $cmd] } {
> > > > warning "lto.exp does not support $cmd in secondary
> > > > source files"
> > > > - } else {
> > > > + } elseif { ![lto-can-handle-directive $op] } {
> > > > # Ignore unrecognized dg- commands, but warn about
> > > > them.
> > > > warning "lto.exp does not support $cmd in secondary
> > > > source files"
> > > > }
> > > > }
> > > >
> > > > + verbose "dg-messages: ${dg-messages}" 3
> > > > + dict append dg-messages-by-file $src ${dg-messages}
> > > > +
> > > > return ${dg-extra-tool-flags}
> > > > }
> > > >
> > > > @@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
> > > > verbose "lto-execute: $src1" 1
> > > > set compile_type "run"
> > > > set dg-do-what [list ${dg-do-what-default} "" P]
> > > > + set dg-messages-by-file [dict create]
> > > > set extra_flags(0) [lto-get-options-main $src1]
> > > > set compile_xfail(0) ""
> > > >
> > > > @@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
> > > > lto-link-and-maybe-run \
> > > > "[lindex $obj_list 0]-[lindex $obj_list
> > > > end]" \
> > > > $obj_list $execname $filtered ${dg-extra-
> > > > ld-
> > > > options} \
> > > > - $filtered
> > > > + $filtered ${dg-messages-by-file}
> > > > }
> > > >
> > > >
> > > > --
> > > > 1.8.5.3
> > > >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
2018-01-08 19:38 ` David Malcolm
@ 2018-01-15 10:09 ` Richard Biener
2018-01-18 12:41 ` Christophe Lyon
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2018-01-15 10:09 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, Jan Hubicka
On Mon, Jan 8, 2018 at 8:36 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Sat, 2018-01-06 at 08:44 +0100, Richard Biener wrote:
>> On January 5, 2018 11:55:11 PM GMT+01:00, David Malcolm <dmalcolm@red
>> hat.com> wrote:
>> > On Fri, 2018-01-05 at 10:36 +0100, Richard Biener wrote:
>> > > On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <dmalcolm@redhat.c
>> > > om>
>> > > wrote:
>> > > > PR lto/83121 reports an ICE deep inside the linemap code when
>> > > > -Wodr
>> > > > reports on a type mismatch.
>> > > >
>> > > > The root cause is that the warning can access the
>> > > > DECL_SOURCE_LOCATION
>> > > > of a streamed-in decl before the lto_location_cache has been
>> > > > applied.
>> > > >
>> > > > lto_location_cache::input_location stores
>> > > > RESERVED_LOCATION_COUNT
>> > > > (==2)
>> > > > as a poison value until the cache is applied:
>> > > > 250 /* Keep value RESERVED_LOCATION_COUNT in *loc as
>> > > > linemap
>> > > > lookups will
>> > > > 251 ICE on it. */
>> > > >
>> > > > The fix is relatively simple: apply the cache before reading
>> > > > the
>> > > > DECL_SOURCE_LOCATION.
>> > > >
>> > > > (I wonder if we should instead have a INVALID_LOCATION value to
>> > > > handle
>> > > > this case more explicitly? e.g. 0xffffffff? or reserve 2 in
>> > > > libcpp for
>> > > > that purpose, and have the non-reserved locations start at
>> > > > 3? Either
>> > > > would be more invasive, though)
>> > > >
>> > > > Triggering the ICE was fiddly: it seems to be affected by many
>> > > > things,
>> > > > including the order of files, and (I think) by filenames. My
>> > > > theory is
>> > > > that it's affected by the ordering of the tree nodes in the LTO
>> > > > stream:
>> > > > for the ICE to occur, the types in question need to be compared
>> > > > before
>> > > > some other operation flushes the lto_location_cache. This
>> > > > ordering
>> > > > is affected by the hash-based ordering in DFS in lto-streamer-
>> > > > out.c, which
>> > > > might explain why r255066 seemed to trigger the bug; the only
>> > > > relevant
>> > > > change to LTO there seemed to be:
>> > > > * lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and
>> > > > DECL_PADDING_P.
>> > > > If so, then the bug was presumably already present, but hidden.
>> > > >
>> > > > The patch also adds regression test coverage for the ICE, which
>> > > > is
>> > > > more
>> > > > involved - as far as I can tell, we don't have an existing way
>> > > > to
>> > > > verify
>> > > > diagnostics emitted during link-time optimization.
>> > > >
>> > > > Hence the patch adds some machinery to lib/lto.exp to support
>> > > > two
>> > > > new
>> > > > directives: dg-lto-warning and dg-lto-message, corresponding to
>> > > > dg-warning and dg-message respectively, where the diagnostics
>> > > > are
>> > > > expected to be emitted at link-time.
>> > > >
>> > > > The test case includes examples of LTO warnings and notes in
>> > > > both
>> > > > the
>> > > > primary and secondary source files
>> > > >
>> > > > Doing so required reusing the logic from DejaGnu for handling
>> > > > diagnostics.
>> > > > Unfortunately the pertinent code is a 50 line loop within a
>> > > > ~200
>> > > > line Tcl
>> > > > function in dg.exp (dg-test), so I had to copy it from DejaGnu,
>> > > > making
>> > > > various changes as necessary (see
>> > > > lto_handle_diagnostics_for_file
>> > > > in the
>> > > > patch; for example the LTO version supports multiple source
>> > > > files,
>> > > > identifying which source file emitted a diagnostic).
>> > > >
>> > > > For non-LTO diagnostics we currently ignore surplus "note"
>> > > > diagnostics.
>> > > > This patch updates lto_prune_warns to follow this behavior
>> > > > (since
>> > > > otherwise we'd need numerous dg-lto-message directives for the
>> > > > motivating
>> > > > test case).
>> > > >
>> > > > The patch adds these PASS results to g++.sum:
>> > > >
>> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
>> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C
>> > > > line
>> > > > 6)
>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C
>> > > > line
>> > > > 8)
>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C
>> > > > line
>> > > > 2)
>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C
>> > > > line
>> > > > 3)
>> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o
>> > > > link, -O0 -flto
>> > > >
>> > > > The output for dg-lto-message above refers to "warnings",
>> > > > rather
>> > > > than
>> > > > "messages" but that's the same as for the non-LTO case, where
>> > > > dg-
>> > > > message
>> > > > also refers to "warnings".
>> > > >
>> > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
>> > > >
>> > > > OK for trunk?
>> > >
>> > > Hmm, but we do this in warn_odr already? How's that not enough?
>> > >
>> > > At least it seems the place you add this isn't ideal (not at the
>> > > "root cause").
>> > >
>> > > Richard.
>> >
>> > [CCing Honza]
>> >
>> > Yes, warn_odr does apply the cache, but this warning is coming from
>> > warn_types_mismatch.
>> >
>> > Of the various calls to warn_types_mismatch, most are immediately
>> > after
>> > a warn_odr guarded by if (warn && warned) or if (warned), and if
>> > warn_odr writes back true to *warned, it has definitely applied the
>> > cache.
>> >
>> > However, the 7-argument overload of odr_types_equivalent_p can
>> > also call warn_types_mismatch, passing in the location_t values it
>> > received.
>> >
>> > The problem occurs with this call stack, when:
>> >
>> > add_type_duplicate, passes DECL_SOURCE_LOCATIONs to:
>> > odr_types_equivalent_p (7-argument overload) - which calls:
>> > warn_types_mismatch, which calls:
>> > warning_at with the given location_t
>> >
>> > where the DECL_SOURCE_LOCATION might not yet have been fixed up
>> > yet.
>> > My patch adds the fixup there.
>> >
>> > This behavior seems to have been introduced by r224248 (aka
>> > 379ca7f842e5b13109b689b3c732741cab3d178e), Honza's fix for "PR
>> > lto/65378" (though that seems to have been a typo, as that bug is
>> > unrelated to the changes in that commit); the relevant ML post was:
>> > https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00523.html
>> >
>> >
>> > Would you prefer this if I reworked the two-liner into something
>> > like
>> > an "lto_location_cache::ensure_cache_applied" function?
>>
>> No, but put it in warn_types_mismatch?
>
> Hmm... putting it in warn_types_mismatch dosn't fix it, for the same
> reason as in the earlier description (or maybe I'm misunderstanding
> you?).
>
> As I understand it, the lto_location_cache effectively contains a vec
> of location_t * and expanded locations, and writes back to the
> location_t * once those "expanded" locations can be turned back into
> valid location_t values.
>
> The issue is that add_type_duplicate takes the DECL_SOURCE_LOCATION of
> a decl before the cache is applied, and that location_t gets passed
> around as a parameter. At that point, even if the cache is applied and
> the DECL_SOURCE_LOCATION is fixed, the local location_t values being
> passed around aren't going to be touched (they're not in the lto
> location cache - they're on the stack or in registers).
>
> FWIW, the backtrace at the point of failure looks like as follows; the
> invalid loc=2 values can be seen there. Note how the invalid
> location_t value of 2 is read in frame 17, and is passed around as a
> param in frames 16 down to 10, leading to the crash:
>
> (gdb) bt
> #0 fancy_abort (file=0x2135f00 "../../src/libcpp/line-map.c", line=990,
> function=0x2136390 <linemap_ordinary_map_lookup(line_maps*, unsigned int)::__FUNCTION__> "linemap_ordinary_map_lookup") at ../../src/gcc/diagnostic.c:1500
> #1 0x0000000001c262c7 in linemap_ordinary_map_lookup (line=2, set=<optimized out>)
> at ../../src/libcpp/line-map.c:990
> #2 linemap_lookup (set=set@entry=0x7ffff7ffb000, line=<optimized out>, line@entry=2)
> at ../../src/libcpp/line-map.c:943
> #3 0x0000000001c27294 in linemap_macro_loc_to_def_point (original_map=0x7fffffffd2e0, location=2,
> set=0x7ffff7ffb000) at ../../src/libcpp/line-map.c:1448
> #4 linemap_resolve_location (set=0x7ffff7ffb000, loc=<optimized out>, lrk=<optimized out>, map=0x7fffffffd2e0)
> at ../../src/libcpp/line-map.c:1580
> #5 0x0000000001bdc4f2 in diagnostic_report_current_module (context=0x288bce0 <global_diagnostic_context>, where=2)
> at ../../src/gcc/diagnostic.c:581
> #6 0x0000000000f36ae0 in diagnostic_report_current_function (context=0x288bce0 <global_diagnostic_context>,
> diagnostic=0x7fffffffd3f0) at ../../src/gcc/tree-diagnostic.c:39
> #7 0x0000000000f36b44 in default_tree_diagnostic_starter (context=0x288bce0 <global_diagnostic_context>,
> diagnostic=0x7fffffffd3f0) at ../../src/gcc/tree-diagnostic.c:48
> #8 0x0000000001bdd3b4 in diagnostic_report_diagnostic (context=0x288bce0 <global_diagnostic_context>,
> diagnostic=0x7fffffffd3f0) at ../../src/gcc/diagnostic.c:985
> #9 0x0000000001bdd90e in diagnostic_impl (richloc=0x7fffffffd450, opt=-1,
> gmsgid=0x1cdcd40 "array types have different bounds", ap=0x7fffffffd4d8, kind=DK_NOTE)
> at ../../src/gcc/diagnostic.c:1108
> #10 0x0000000001bddbfa in inform (location=2, gmsgid=0x1cdcd40 "array types have different bounds")
> at ../../src/gcc/diagnostic.c:1160
> #11 0x0000000000bb2dd0 in warn_types_mismatch (t1=<array_type 0x7ffff1aa6dc8>, t2=<array_type 0x7ffff1aa69d8>,
> loc1=2, loc2=4448) at ../../src/gcc/ipa-devirt.c:1176
> #12 0x0000000000bb5245 in odr_types_equivalent_p (t1=<record_type 0x7ffff1aa6e70 AsyncHooks>,
> t2=<record_type 0x7ffff1aa6b28 AsyncHooks>, warn=true, warned=0x7fffffffd7e3, visited=0x7fffffffd7b0, loc1=2,
> loc2=4448) at ../../src/gcc/ipa-devirt.c:1569
> #13 0x0000000000bb6ebb in add_type_duplicate (val=0x7ffff18ab570, type=<record_type 0x7ffff1aa6b28 AsyncHooks>)
> at ../../src/gcc/ipa-devirt.c:1848
> #14 0x0000000000bb77f2 in get_odr_type (type=<record_type 0x7ffff1aa6b28 AsyncHooks>, insert=true)
> at ../../src/gcc/ipa-devirt.c:2028
> #15 0x0000000000bb0ae0 in odr_subtypes_equivalent_p (t1=<record_type 0x7ffff1aa6b28 AsyncHooks>,
> t2=<record_type 0x7ffff1aa6e70 AsyncHooks>, visited=0x7fffffffda10, loc1=288, loc2=2)
> at ../../src/gcc/ipa-devirt.c:689
> #16 0x0000000000bb5151 in odr_types_equivalent_p (t1=<record_type 0x7ffff1aa6a80 Environment>,
> t2=<record_type 0x7ffff1aa6f18 Environment>, warn=true, warned=0x7fffffffda43, visited=0x7fffffffda10, loc1=288,
> loc2=2) at ../../src/gcc/ipa-devirt.c:1556
> #17 0x0000000000bb6ebb in add_type_duplicate (val=0x7ffff18ab4e0, type=<record_type 0x7ffff1aa6f18 Environment>)
> at ../../src/gcc/ipa-devirt.c:1848
> #18 0x0000000000bb77f2 in get_odr_type (type=<record_type 0x7ffff1aa6f18 Environment>, insert=true)
> at ../../src/gcc/ipa-devirt.c:2028
> #19 0x0000000000bb7e42 in register_odr_type (type=<record_type 0x7ffff1aa6f18 Environment>)
> at ../../src/gcc/ipa-devirt.c:2111
> #20 0x00000000007eae64 in lto_read_decls (decl_data=0x7ffff1696000, data=0x296c040, resolutions=...)
> at ../../src/gcc/lto/lto.c:1755
> #21 0x00000000007eb9c3 in lto_file_finalize (file_data=0x7ffff1696000, file=0x2968cc0)
> at ../../src/gcc/lto/lto.c:2055
> #22 0x00000000007eba1b in lto_create_files_from_ids (file=0x2968cc0, file_data=0x7ffff1696000, count=0x7fffffffdd4c)
> at ../../src/gcc/lto/lto.c:2065
> #23 0x00000000007ebb4f in lto_file_read (file=0x2968cc0, resolution_file=0x290aad0, count=0x7fffffffdd4c)
> at ../../src/gcc/lto/lto.c:2106
> #24 0x00000000007ef930 in read_cgraph_and_symbols (nfiles=2, fnames=0x28e5c00) at ../../src/gcc/lto/lto.c:2818
> #25 0x00000000007f0c16 in lto_main () at ../../src/gcc/lto/lto.c:3323
> #26 0x0000000000eb3ae6 in compile_file () at ../../src/gcc/toplev.c:455
> #27 0x0000000000eb6de6 in do_compile () at ../../src/gcc/toplev.c:2076
> #28 0x0000000000eb715c in toplev::main (this=0x7fffffffdeae, argc=16, argv=0x28a0aa0) at ../../src/gcc/toplev.c:2211
> #29 0x0000000001bbd183 in main (argc=15, argv=0x7fffffffdfa8) at ../../src/gcc/main.c:39
>
> My patch put the apply_location_cache immediately before the read in
> add_type_duplicate, but maybe there's a better place?
Ugh, quite a maze ;) So your original patch is ok unless Honza comes
up with something better.
Thanks,
Richard.
> Thanks
> Dave
>
>>
>> Richard.
>>
>> > Thanks
>> > Dave
>> >
>> > >
>> > > > gcc/ChangeLog:
>> > > > PR lto/83121
>> > > > * ipa-devirt.c (add_type_duplicate): When comparing
>> > > > memory
>> > > > layout,
>> > > > call the lto_location_cache before reading the
>> > > > DECL_SOURCE_LOCATION of the types.
>> > > >
>> > > > gcc/testsuite/ChangeLog:
>> > > > PR lto/83121
>> > > > * g++.dg/lto/pr83121_0.C: New test case.
>> > > > * g++.dg/lto/pr83121_1.C: New test case.
>> > > > * lib/lto.exp (lto_handle_diagnostics_for_file): New
>> > > > procedure,
>> > > > adapted from DejaGnu's dg-test.
>> > > > (lto_handle_diagnostics): New procedure.
>> > > > (lto_prune_warns): Ignore informational notes.
>> > > > (lto-link-and-maybe-run): Add "messages_by_file" param.
>> > > > Call lto_handle_diagnostics. Avoid issuing
>> > > > "unresolved"
>> > > > for
>> > > > "execute" when "link" fails if "execute" was not
>> > > > specified.
>> > > > (lto-can-handle-directive): New procedure.
>> > > > (lto-get-options-main): Call lto-can-handle-
>> > > > directive. Add
>> > > > a
>> > > > dg-messages local, using it to set the caller's
>> > > > dg-messages-by-file for the given source file.
>> > > > (lto-get-options): Likewise.
>> > > > (lto-execute): Add dg-messages-by-file local, and pass
>> > > > it
>> > > > to
>> > > > lto-link-and-maybe-run.
>> > > > ---
>> > > > gcc/ipa-devirt.c | 7 +-
>> > > > gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
>> > > > gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
>> > > > gcc/testsuite/lib/lto.exp | 199
>> > > > ++++++++++++++++++++++++++++++++++-
>> > > > 4 files changed, 222 insertions(+), 6 deletions(-)
>> > > > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > > > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > > >
>> > > > diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
>> > > > index 540f038..f3d2e4a 100644
>> > > > --- a/gcc/ipa-devirt.c
>> > > > +++ b/gcc/ipa-devirt.c
>> > > > @@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree
>> > > > type)
>> > > > }
>> > > > }
>> > > >
>> > > > - /* Next compare memory layout. */
>> > > > + /* Next compare memory layout.
>> > > > + The DECL_SOURCE_LOCATIONs in this invocation came from
>> > > > LTO
>> > > > streaming.
>> > > > + We must apply the location cache to ensure that they are
>> > > > valid
>> > > > + before we can pass them to odr_types_equivalent_p (PR
>> > > > lto/83121). */
>> > > > + if (lto_location_cache::current_cache)
>> > > > + lto_location_cache::current_cache->apply_location_cache
>> > > > ();
>> > > > if (!odr_types_equivalent_p (val->type, type,
>> > > > !flag_ltrans && !val-
>> > > > >odr_violated
>> > > > && !warned,
>> > > > &warned, &visited,
>> > > > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > > > b/gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > > > new file mode 100644
>> > > > index 0000000..ef894c7
>> > > > --- /dev/null
>> > > > +++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
>> > > > @@ -0,0 +1,12 @@
>> > > > +// { dg-lto-do link }
>> > > > +// { dg-lto-options {{-O0 -flto}} }
>> > > > +/* We need -O0 to avoid the "Environment" locals in the test
>> > > > functions
>> > > > + from being optimized away. */
>> > > > +
>> > > > +struct Environment { // { dg-lto-warning "8: type 'struct
>> > > > Environment' violates the C\\+\\+ One Definition Rule" }
>> > > > + struct AsyncHooks {
>> > > > + int providers_[2]; // { dg-lto-message "a field of same
>> > > > name
>> > > > but different type is defined in another translation unit" }
>> > > > + };
>> > > > + AsyncHooks async_hooks_;
>> > > > +};
>> > > > +void fn2() { Environment a; }
>> > > > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > > > b/gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > > > new file mode 100644
>> > > > index 0000000..2aef1b5
>> > > > --- /dev/null
>> > > > +++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
>> > > > @@ -0,0 +1,10 @@
>> > > > +struct Environment {
>> > > > + struct AsyncHooks { // { dg-lto-warning "10: type 'struct
>> > > > AsyncHooks' violates the C\\+\\+ One Definition Rule" }
>> > > > + int providers_[1]; // { dg-lto-message "the first
>> > > > difference
>> > > > of corresponding definitions is field 'providers_'" }
>> > > > + };
>> > > > + AsyncHooks async_hooks_;
>> > > > +};
>> > > > +void fn1() { Environment a; }
>> > > > +int main ()
>> > > > +{
>> > > > +}
>> > > > diff --git a/gcc/testsuite/lib/lto.exp
>> > > > b/gcc/testsuite/lib/lto.exp
>> > > > index 477f683..e69d8d4 100644
>> > > > --- a/gcc/testsuite/lib/lto.exp
>> > > > +++ b/gcc/testsuite/lib/lto.exp
>> > > > @@ -16,6 +16,122 @@
>> > > >
>> > > > # Contributed by Diego Novillo <dnovillo@google.com>
>> > > >
>> > > > +# A subroutine of lto_handle_diagnostics: check TEXT for the
>> > > > expected
>> > > > +# diagnostics for one specific source file, issuing PASS/FAIL
>> > > > results.
>> > > > +# Return TEXT, stripped of any diagnostics that were handled.
>> > > > +#
>> > > > +# NAME is the testcase name to use when reporting PASS/FAIL
>> > > > results.
>> > > > +# FILENAME is the name (with full path) of the file we're
>> > > > interested in.
>> > > > +# MESSAGES_FOR_FILE is a list of expected messages, akin to
>> > > > DejaGnu's
>> > > > +# "dg-messages" variable.
>> > > > +# TEXT is the textual output from the LTO link.
>> > > > +
>> > > > +proc lto_handle_diagnostics_for_file { name filename
>> > > > messages_for_file text } {
>> > > > + global dg-linenum-format
>> > > > +
>> > > > + set filename_without_path [file tail $filename]
>> > > > +
>> > > > + # This loop is adapted from the related part of DejaGnu's
>> > > > dg-
>> > > > test,
>> > > > + # with changes as detailed below to cope with the LTO
>> > > > case.
>> > > > +
>> > > > + foreach i ${messages_for_file} {
>> > > > + verbose "Scanning for message: $i" 4
>> > > > +
>> > > > + # Remove all error messages for the line [lindex $i 0]
>> > > > + # in the source file. If we find any, success!
>> > > > + set line [lindex $i 0]
>> > > > + set pattern [lindex $i 2]
>> > > > + set comment [lindex $i 3]
>> > > > + verbose "line: $line" 4
>> > > > + verbose "pattern: $pattern" 4
>> > > > + verbose "comment: $comment" 4
>> > > > + #send_user "Before:\n$text\n"
>> > > > +
>> > > > + # Unlike dg-test, we use $filename_without_path in this
>> > > > pattern.
>> > > > + # This is to ensure that we have the correct file/line
>> > > > combination.
>> > > > + # This imposes the restriction that the filename can't
>> > > > contain
>> > > > + # any regexp control characters. We have to strip the
>> > > > path, since
>> > > > + # e.g. the '+' in "g++.dg" wouldn't be valid.
>> > > > + set pat
>> > > > "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[
>> > > > ^\n\
>> > > > ]*\n?)+"
>> > > > + if {[regsub -all $pat $text "\n" text]} {
>> > > > + set text [string trimleft $text]
>> > > > + set ok pass
>> > > > + set uhoh fail
>> > > > + } else {
>> > > > + set ok fail
>> > > > + set uhoh pass
>> > > > + }
>> > > > + #send_user "After:\n$text\n"
>> > > > +
>> > > > + # $line will either be a formatted line number or a
>> > > > number
>> > > > all by
>> > > > + # itself. Delete the formatting.
>> > > > + scan $line ${dg-linenum-format} line
>> > > > +
>> > > > + # Unlike dg-test, add the filename to the PASS/FAIL
>> > > > message
>> > > > (rather
>> > > > + # than just the line number) so that the user can
>> > > > identify
>> > > > the
>> > > > + # pertinent directive.
>> > > > + set describe_where "$filename_without_path line $line"
>> > > > +
>> > > > + # Issue the PASS/FAIL, adding "LTO" to the messages
>> > > > (e.g.
>> > > > "LTO errors")
>> > > > + # to distinguish them from the non-LTO case (in case we
>> > > > ever need to
>> > > > + # support both).
>> > > > + switch [lindex $i 1] {
>> > > > + "ERROR" {
>> > > > + $ok "$name $comment (test for LTO errors,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "XERROR" {
>> > > > + x$ok "$name $comment (test for LTO errors,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "WARNING" {
>> > > > + $ok "$name $comment (test for LTO warnings,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "XWARNING" {
>> > > > + x$ok "$name $comment (test for LTO warnings,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "BOGUS" {
>> > > > + $uhoh "$name $comment (test for LTO bogus
>> > > > messages,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "XBOGUS" {
>> > > > + x$uhoh "$name $comment (test for LTO bogus
>> > > > messages, $describe_where)"
>> > > > + }
>> > > > + "BUILD" {
>> > > > + $uhoh "$name $comment (test for LTO build
>> > > > failure,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "XBUILD" {
>> > > > + x$uhoh "$name $comment (test for LTO build
>> > > > failure,
>> > > > $describe_where)"
>> > > > + }
>> > > > + "EXEC" { }
>> > > > + "XEXEC" { }
>> > > > + }
>> > > > + }
>> > > > + return $text
>> > > > +}
>> > > > +
>> > > > +# Support for checking for link-time diagnostics: check for
>> > > > +# the expected diagnostics within TEXT, issuing PASS/FAIL
>> > > > results.
>> > > > +# Return TEXT, stripped of any diagnostics that were handled.
>> > > > +#
>> > > > +# MESSAGES_BY_FILE is a dict; the keys are source files (with
>> > > > paths)
>> > > > +# the values are lists of expected messages, akin to DejaGnu's
>> > > > "dg-messages"
>> > > > +# variable.
>> > > > +# TEXT is the textual output from the LTO link.
>> > > > +
>> > > > +proc lto_handle_diagnostics { messages_by_file text } {
>> > > > + global testcase
>> > > > +
>> > > > + verbose "lto_handle_diagnostics: entry: $text" 2
>> > > > + verbose " messages_by_file $messages_by_file" 3
>> > > > +
>> > > > + dict for {src dg-messages} $messages_by_file {
>> > > > + set text [lto_handle_diagnostics_for_file $testcase
>> > > > $src \
>> > > > + ${dg-messages} $text]
>> > > > + }
>> > > > +
>> > > > + verbose "lto_handle_diagnostics: exit: $text" 2
>> > > > +
>> > > > + return $text
>> > > > +}
>> > > > +
>> > > > # Prune messages that aren't useful.
>> > > >
>> > > > proc lto_prune_warns { text } {
>> > > > @@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
>> > > > regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]*
>> > > > value=\[^\n\]*;
>> > > > file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
>> > > > regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken"
>> > > > $text ""
>> > > > text
>> > > >
>> > > > + # Ignore informational notes.
>> > > > + regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
>> > > > +
>> > > > verbose "lto_prune_warns: exit: $text" 2
>> > > >
>> > > > return $text
>> > > > @@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile
>> > > > optstr xfaildata } {
>> > > > # OPTALL is a list of compiler and linker options to use for
>> > > > all
>> > > > tests
>> > > > # OPTFILE is a list of compiler and linker options to use for
>> > > > this
>> > > > test
>> > > > # OPTSTR is the list of options to list in messages
>> > > > -proc lto-link-and-maybe-run { testname objlist dest optall
>> > > > optfile
>> > > > optstr } {
>> > > > +# MESSAGES_BY_FILE is a dict of (src, dg-messages)
>> > > > +proc lto-link-and-maybe-run { testname objlist dest optall
>> > > > optfile
>> > > > optstr \
>> > > > + messages_by_file } {
>> > > > global testcase
>> > > > global tool
>> > > > global compile_type
>> > > > global board_info
>> > > >
>> > > > + verbose "lto-link-and-maybe-run" 2
>> > > > + verbose " messages_by_file $messages_by_file" 3
>> > > > +
>> > > > # Check that all of the objects were built successfully.
>> > > > foreach obj [split $objlist] {
>> > > > if ![file_on_host exists $obj] then {
>> > > > @@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname
>> > > > objlist dest optall optfile optstr } {
>> > > > set board_info($target_board,ldscript) $saved_ldscript
>> > > > }
>> > > >
>> > > > + # Check for diagnostics specified by directives
>> > > > + set comp_output [lto_handle_diagnostics $messages_by_file
>> > > > $comp_output]
>> > > > +
>> > > > # Prune unimportant visibility warnings before checking
>> > > > output.
>> > > > set comp_output [lto_prune_warns $comp_output]
>> > > >
>> > > > if ![${tool}_check_compile "$testcase $testname link"
>> > > > $optstr
>> > > > \
>> > > > $dest $comp_output] then {
>> > > > - unresolved "$testcase $testname execute $optstr"
>> > > > + if { ![string compare "execute" $compile_type] } {
>> > > > + unresolved "$testcase $testname execute $optstr"
>> > > > + }
>> > > > return
>> > > > }
>> > > >
>> > > > @@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname
>> > > > objlist
>> > > > dest optall optfile optstr } {
>> > > > $status "$testcase $testname execute $optstr"
>> > > > }
>> > > >
>> > > > +# Potentially handle the given dg- directive (a list)
>> > > > +# Return true is the directive was handled, false otherwise.
>> > > > +
>> > > > +proc lto-can-handle-directive { op } {
>> > > > + set cmd [lindex $op 0]
>> > > > +
>> > > > + # dg-warning and dg-message append to dg-messages.
>> > > > + upvar dg-messages dg-messages
>> > > > +
>> > > > + # A list of directives to recognize, and a list of
>> > > > directives
>> > > > + # to remap them to.
>> > > > + # For example, "dg-lto-warning" is implemented by calling
>> > > > "dg-
>> > > > warning".
>> > > > + set directives { dg-lto-warning dg-lto-message }
>> > > > + set remapped_directives { dg-warning dg-message }
>> > > > +
>> > > > + set idx [lsearch -exact $directives $cmd]
>> > > > + if { $idx != -1 } {
>> > > > + verbose "remapping from: $op" 4
>> > > > +
>> > > > + set remapped_cmd [lindex $remapped_directives $idx]
>> > > > + set op [lreplace $op 0 0 $remapped_cmd]
>> > > > +
>> > > > + verbose "remapped to: $op" 4
>> > > > +
>> > > > + set status [catch "$op" errmsg]
>> > > > + if { $status != 0 } {
>> > > > + if { 0 && [info exists errorInfo] } {
>> > > > + # This also prints a backtrace which will just
>> > > > confuse
>> > > > + # testcase writers, so it's disabled.
>> > > > + perror "$name: $errorInfo\n"
>> > > > + } else {
>> > > > + perror "$name: $errmsg for \"$op\"\n"
>> > > > + }
>> > > > + # ??? The call to unresolved here is necessary to
>> > > > clear
>> > > > `errcnt'.
>> > > > + # What we really need is a proc like perror that
>> > > > doesn't set errcnt.
>> > > > + # It should also set exit_status to 1.
>> > > > + unresolved "$name: $errmsg for \"$op\""
>> > > > + }
>> > > > +
>> > > > + return true
>> > > > + }
>> > > > +
>> > > > + return false
>> > > > +}
>> > > > +
>> > > > # lto-get-options-main -- get target requirements for a test
>> > > > and
>> > > > # options for the primary source file and the test as a whole
>> > > > #
>> > > > @@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
>> > > > upvar dg-final-code dg-final-code
>> > > > set dg-final-code ""
>> > > >
>> > > > + # dg-warning and dg-message append to dg-messages.
>> > > > + upvar dg-messages-by-file dg-messages-by-file
>> > > > + set dg-messages ""
>> > > > +
>> > > > set tmp [dg-get-options $src]
>> > > > verbose "getting options for $src: $tmp"
>> > > > foreach op $tmp {
>> > > > @@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
>> > > > } else {
>> > > > append dg-final-code "[lindex $op 2]\n"
>> > > > }
>> > > > - } else {
>> > > > + } elseif { ![lto-can-handle-directive $op] } {
>> > > > # Ignore unrecognized dg- commands, but warn about
>> > > > them.
>> > > > warning "lto.exp does not support $cmd"
>> > > > }
>> > > > }
>> > > >
>> > > > + verbose "dg-messages: ${dg-messages}" 3
>> > > > + dict append dg-messages-by-file $src ${dg-messages}
>> > > > +
>> > > > # Return flags to use for compiling the primary source
>> > > > file
>> > > > and for
>> > > > # linking.
>> > > > verbose "dg-extra-tool-flags for main is ${dg-extra-tool-
>> > > > flags}"
>> > > > @@ -373,6 +554,10 @@ proc lto-get-options { src } {
>> > > > # dg-xfail-if needs access to dg-do-what.
>> > > > upvar dg-do-what dg-do-what
>> > > >
>> > > > + # dg-warning appends to dg-messages.
>> > > > + upvar dg-messages-by-file dg-messages-by-file
>> > > > + set dg-messages ""
>> > > > +
>> > > > set tmp [dg-get-options $src]
>> > > > foreach op $tmp {
>> > > > set cmd [lindex $op 0]
>> > > > @@ -386,12 +571,15 @@ proc lto-get-options { src } {
>> > > > }
>> > > > } elseif { [string match "dg-require-*" $cmd] } {
>> > > > warning "lto.exp does not support $cmd in secondary
>> > > > source files"
>> > > > - } else {
>> > > > + } elseif { ![lto-can-handle-directive $op] } {
>> > > > # Ignore unrecognized dg- commands, but warn about
>> > > > them.
>> > > > warning "lto.exp does not support $cmd in secondary
>> > > > source files"
>> > > > }
>> > > > }
>> > > >
>> > > > + verbose "dg-messages: ${dg-messages}" 3
>> > > > + dict append dg-messages-by-file $src ${dg-messages}
>> > > > +
>> > > > return ${dg-extra-tool-flags}
>> > > > }
>> > > >
>> > > > @@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
>> > > > verbose "lto-execute: $src1" 1
>> > > > set compile_type "run"
>> > > > set dg-do-what [list ${dg-do-what-default} "" P]
>> > > > + set dg-messages-by-file [dict create]
>> > > > set extra_flags(0) [lto-get-options-main $src1]
>> > > > set compile_xfail(0) ""
>> > > >
>> > > > @@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
>> > > > lto-link-and-maybe-run \
>> > > > "[lindex $obj_list 0]-[lindex $obj_list
>> > > > end]" \
>> > > > $obj_list $execname $filtered ${dg-extra-
>> > > > ld-
>> > > > options} \
>> > > > - $filtered
>> > > > + $filtered ${dg-messages-by-file}
>> > > > }
>> > > >
>> > > >
>> > > > --
>> > > > 1.8.5.3
>> > > >
>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
2018-01-15 10:09 ` Richard Biener
@ 2018-01-18 12:41 ` Christophe Lyon
0 siblings, 0 replies; 7+ messages in thread
From: Christophe Lyon @ 2018-01-18 12:41 UTC (permalink / raw)
To: Richard Biener; +Cc: David Malcolm, GCC Patches, Jan Hubicka
Hi David,
On 15 January 2018 at 11:09, Richard Biener <richard.guenther@gmail.com> wrote:
> On Mon, Jan 8, 2018 at 8:36 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> On Sat, 2018-01-06 at 08:44 +0100, Richard Biener wrote:
>>> On January 5, 2018 11:55:11 PM GMT+01:00, David Malcolm <dmalcolm@red
>>> hat.com> wrote:
>>> > On Fri, 2018-01-05 at 10:36 +0100, Richard Biener wrote:
>>> > > On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <dmalcolm@redhat.c
>>> > > om>
>>> > > wrote:
>>> > > > PR lto/83121 reports an ICE deep inside the linemap code when
>>> > > > -Wodr
>>> > > > reports on a type mismatch.
>>> > > >
>>> > > > The root cause is that the warning can access the
>>> > > > DECL_SOURCE_LOCATION
>>> > > > of a streamed-in decl before the lto_location_cache has been
>>> > > > applied.
>>> > > >
>>> > > > lto_location_cache::input_location stores
>>> > > > RESERVED_LOCATION_COUNT
>>> > > > (==2)
>>> > > > as a poison value until the cache is applied:
>>> > > > 250 /* Keep value RESERVED_LOCATION_COUNT in *loc as
>>> > > > linemap
>>> > > > lookups will
>>> > > > 251 ICE on it. */
>>> > > >
>>> > > > The fix is relatively simple: apply the cache before reading
>>> > > > the
>>> > > > DECL_SOURCE_LOCATION.
>>> > > >
>>> > > > (I wonder if we should instead have a INVALID_LOCATION value to
>>> > > > handle
>>> > > > this case more explicitly? e.g. 0xffffffff? or reserve 2 in
>>> > > > libcpp for
>>> > > > that purpose, and have the non-reserved locations start at
>>> > > > 3? Either
>>> > > > would be more invasive, though)
>>> > > >
>>> > > > Triggering the ICE was fiddly: it seems to be affected by many
>>> > > > things,
>>> > > > including the order of files, and (I think) by filenames. My
>>> > > > theory is
>>> > > > that it's affected by the ordering of the tree nodes in the LTO
>>> > > > stream:
>>> > > > for the ICE to occur, the types in question need to be compared
>>> > > > before
>>> > > > some other operation flushes the lto_location_cache. This
>>> > > > ordering
>>> > > > is affected by the hash-based ordering in DFS in lto-streamer-
>>> > > > out.c, which
>>> > > > might explain why r255066 seemed to trigger the bug; the only
>>> > > > relevant
>>> > > > change to LTO there seemed to be:
>>> > > > * lto-streamer-out.c (hash_tree): Hash TYPE_EMPTY_P and
>>> > > > DECL_PADDING_P.
>>> > > > If so, then the bug was presumably already present, but hidden.
>>> > > >
>>> > > > The patch also adds regression test coverage for the ICE, which
>>> > > > is
>>> > > > more
>>> > > > involved - as far as I can tell, we don't have an existing way
>>> > > > to
>>> > > > verify
>>> > > > diagnostics emitted during link-time optimization.
>>> > > >
>>> > > > Hence the patch adds some machinery to lib/lto.exp to support
>>> > > > two
>>> > > > new
>>> > > > directives: dg-lto-warning and dg-lto-message, corresponding to
>>> > > > dg-warning and dg-message respectively, where the diagnostics
>>> > > > are
>>> > > > expected to be emitted at link-time.
>>> > > >
>>> > > > The test case includes examples of LTO warnings and notes in
>>> > > > both
>>> > > > the
>>> > > > primary and secondary source files
>>> > > >
>>> > > > Doing so required reusing the logic from DejaGnu for handling
>>> > > > diagnostics.
>>> > > > Unfortunately the pertinent code is a 50 line loop within a
>>> > > > ~200
>>> > > > line Tcl
>>> > > > function in dg.exp (dg-test), so I had to copy it from DejaGnu,
>>> > > > making
>>> > > > various changes as necessary (see
>>> > > > lto_handle_diagnostics_for_file
>>> > > > in the
>>> > > > patch; for example the LTO version supports multiple source
>>> > > > files,
>>> > > > identifying which source file emitted a diagnostic).
>>> > > >
>>> > > > For non-LTO diagnostics we currently ignore surplus "note"
>>> > > > diagnostics.
>>> > > > This patch updates lto_prune_warns to follow this behavior
>>> > > > (since
>>> > > > otherwise we'd need numerous dg-lto-message directives for the
>>> > > > motivating
>>> > > > test case).
>>> > > >
>>> > > > The patch adds these PASS results to g++.sum:
>>> > > >
>>> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o assemble, -O0 -flto
>>> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_1.o assemble, -O0 -flto
>>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C
>>> > > > line
>>> > > > 6)
>>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C
>>> > > > line
>>> > > > 8)
>>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C
>>> > > > line
>>> > > > 2)
>>> > > > PASS: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C
>>> > > > line
>>> > > > 3)
>>> > > > PASS: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o
>>> > > > link, -O0 -flto
>>> > > >
>>> > > > The output for dg-lto-message above refers to "warnings",
>>> > > > rather
>>> > > > than
>>> > > > "messages" but that's the same as for the non-LTO case, where
>>> > > > dg-
>>> > > > message
>>> > > > also refers to "warnings".
>>> > > >
>>> > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
>>> > > >
>>> > > > OK for trunk?
>>> > >
>>> > > Hmm, but we do this in warn_odr already? How's that not enough?
>>> > >
>>> > > At least it seems the place you add this isn't ideal (not at the
>>> > > "root cause").
>>> > >
>>> > > Richard.
>>> >
>>> > [CCing Honza]
>>> >
>>> > Yes, warn_odr does apply the cache, but this warning is coming from
>>> > warn_types_mismatch.
>>> >
>>> > Of the various calls to warn_types_mismatch, most are immediately
>>> > after
>>> > a warn_odr guarded by if (warn && warned) or if (warned), and if
>>> > warn_odr writes back true to *warned, it has definitely applied the
>>> > cache.
>>> >
>>> > However, the 7-argument overload of odr_types_equivalent_p can
>>> > also call warn_types_mismatch, passing in the location_t values it
>>> > received.
>>> >
>>> > The problem occurs with this call stack, when:
>>> >
>>> > add_type_duplicate, passes DECL_SOURCE_LOCATIONs to:
>>> > odr_types_equivalent_p (7-argument overload) - which calls:
>>> > warn_types_mismatch, which calls:
>>> > warning_at with the given location_t
>>> >
>>> > where the DECL_SOURCE_LOCATION might not yet have been fixed up
>>> > yet.
>>> > My patch adds the fixup there.
>>> >
>>> > This behavior seems to have been introduced by r224248 (aka
>>> > 379ca7f842e5b13109b689b3c732741cab3d178e), Honza's fix for "PR
>>> > lto/65378" (though that seems to have been a typo, as that bug is
>>> > unrelated to the changes in that commit); the relevant ML post was:
>>> > https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00523.html
>>> >
>>> >
>>> > Would you prefer this if I reworked the two-liner into something
>>> > like
>>> > an "lto_location_cache::ensure_cache_applied" function?
>>>
>>> No, but put it in warn_types_mismatch?
>>
>> Hmm... putting it in warn_types_mismatch dosn't fix it, for the same
>> reason as in the earlier description (or maybe I'm misunderstanding
>> you?).
>>
>> As I understand it, the lto_location_cache effectively contains a vec
>> of location_t * and expanded locations, and writes back to the
>> location_t * once those "expanded" locations can be turned back into
>> valid location_t values.
>>
>> The issue is that add_type_duplicate takes the DECL_SOURCE_LOCATION of
>> a decl before the cache is applied, and that location_t gets passed
>> around as a parameter. At that point, even if the cache is applied and
>> the DECL_SOURCE_LOCATION is fixed, the local location_t values being
>> passed around aren't going to be touched (they're not in the lto
>> location cache - they're on the stack or in registers).
>>
>> FWIW, the backtrace at the point of failure looks like as follows; the
>> invalid loc=2 values can be seen there. Note how the invalid
>> location_t value of 2 is read in frame 17, and is passed around as a
>> param in frames 16 down to 10, leading to the crash:
>>
>> (gdb) bt
>> #0 fancy_abort (file=0x2135f00 "../../src/libcpp/line-map.c", line=990,
>> function=0x2136390 <linemap_ordinary_map_lookup(line_maps*, unsigned int)::__FUNCTION__> "linemap_ordinary_map_lookup") at ../../src/gcc/diagnostic.c:1500
>> #1 0x0000000001c262c7 in linemap_ordinary_map_lookup (line=2, set=<optimized out>)
>> at ../../src/libcpp/line-map.c:990
>> #2 linemap_lookup (set=set@entry=0x7ffff7ffb000, line=<optimized out>, line@entry=2)
>> at ../../src/libcpp/line-map.c:943
>> #3 0x0000000001c27294 in linemap_macro_loc_to_def_point (original_map=0x7fffffffd2e0, location=2,
>> set=0x7ffff7ffb000) at ../../src/libcpp/line-map.c:1448
>> #4 linemap_resolve_location (set=0x7ffff7ffb000, loc=<optimized out>, lrk=<optimized out>, map=0x7fffffffd2e0)
>> at ../../src/libcpp/line-map.c:1580
>> #5 0x0000000001bdc4f2 in diagnostic_report_current_module (context=0x288bce0 <global_diagnostic_context>, where=2)
>> at ../../src/gcc/diagnostic.c:581
>> #6 0x0000000000f36ae0 in diagnostic_report_current_function (context=0x288bce0 <global_diagnostic_context>,
>> diagnostic=0x7fffffffd3f0) at ../../src/gcc/tree-diagnostic.c:39
>> #7 0x0000000000f36b44 in default_tree_diagnostic_starter (context=0x288bce0 <global_diagnostic_context>,
>> diagnostic=0x7fffffffd3f0) at ../../src/gcc/tree-diagnostic.c:48
>> #8 0x0000000001bdd3b4 in diagnostic_report_diagnostic (context=0x288bce0 <global_diagnostic_context>,
>> diagnostic=0x7fffffffd3f0) at ../../src/gcc/diagnostic.c:985
>> #9 0x0000000001bdd90e in diagnostic_impl (richloc=0x7fffffffd450, opt=-1,
>> gmsgid=0x1cdcd40 "array types have different bounds", ap=0x7fffffffd4d8, kind=DK_NOTE)
>> at ../../src/gcc/diagnostic.c:1108
>> #10 0x0000000001bddbfa in inform (location=2, gmsgid=0x1cdcd40 "array types have different bounds")
>> at ../../src/gcc/diagnostic.c:1160
>> #11 0x0000000000bb2dd0 in warn_types_mismatch (t1=<array_type 0x7ffff1aa6dc8>, t2=<array_type 0x7ffff1aa69d8>,
>> loc1=2, loc2=4448) at ../../src/gcc/ipa-devirt.c:1176
>> #12 0x0000000000bb5245 in odr_types_equivalent_p (t1=<record_type 0x7ffff1aa6e70 AsyncHooks>,
>> t2=<record_type 0x7ffff1aa6b28 AsyncHooks>, warn=true, warned=0x7fffffffd7e3, visited=0x7fffffffd7b0, loc1=2,
>> loc2=4448) at ../../src/gcc/ipa-devirt.c:1569
>> #13 0x0000000000bb6ebb in add_type_duplicate (val=0x7ffff18ab570, type=<record_type 0x7ffff1aa6b28 AsyncHooks>)
>> at ../../src/gcc/ipa-devirt.c:1848
>> #14 0x0000000000bb77f2 in get_odr_type (type=<record_type 0x7ffff1aa6b28 AsyncHooks>, insert=true)
>> at ../../src/gcc/ipa-devirt.c:2028
>> #15 0x0000000000bb0ae0 in odr_subtypes_equivalent_p (t1=<record_type 0x7ffff1aa6b28 AsyncHooks>,
>> t2=<record_type 0x7ffff1aa6e70 AsyncHooks>, visited=0x7fffffffda10, loc1=288, loc2=2)
>> at ../../src/gcc/ipa-devirt.c:689
>> #16 0x0000000000bb5151 in odr_types_equivalent_p (t1=<record_type 0x7ffff1aa6a80 Environment>,
>> t2=<record_type 0x7ffff1aa6f18 Environment>, warn=true, warned=0x7fffffffda43, visited=0x7fffffffda10, loc1=288,
>> loc2=2) at ../../src/gcc/ipa-devirt.c:1556
>> #17 0x0000000000bb6ebb in add_type_duplicate (val=0x7ffff18ab4e0, type=<record_type 0x7ffff1aa6f18 Environment>)
>> at ../../src/gcc/ipa-devirt.c:1848
>> #18 0x0000000000bb77f2 in get_odr_type (type=<record_type 0x7ffff1aa6f18 Environment>, insert=true)
>> at ../../src/gcc/ipa-devirt.c:2028
>> #19 0x0000000000bb7e42 in register_odr_type (type=<record_type 0x7ffff1aa6f18 Environment>)
>> at ../../src/gcc/ipa-devirt.c:2111
>> #20 0x00000000007eae64 in lto_read_decls (decl_data=0x7ffff1696000, data=0x296c040, resolutions=...)
>> at ../../src/gcc/lto/lto.c:1755
>> #21 0x00000000007eb9c3 in lto_file_finalize (file_data=0x7ffff1696000, file=0x2968cc0)
>> at ../../src/gcc/lto/lto.c:2055
>> #22 0x00000000007eba1b in lto_create_files_from_ids (file=0x2968cc0, file_data=0x7ffff1696000, count=0x7fffffffdd4c)
>> at ../../src/gcc/lto/lto.c:2065
>> #23 0x00000000007ebb4f in lto_file_read (file=0x2968cc0, resolution_file=0x290aad0, count=0x7fffffffdd4c)
>> at ../../src/gcc/lto/lto.c:2106
>> #24 0x00000000007ef930 in read_cgraph_and_symbols (nfiles=2, fnames=0x28e5c00) at ../../src/gcc/lto/lto.c:2818
>> #25 0x00000000007f0c16 in lto_main () at ../../src/gcc/lto/lto.c:3323
>> #26 0x0000000000eb3ae6 in compile_file () at ../../src/gcc/toplev.c:455
>> #27 0x0000000000eb6de6 in do_compile () at ../../src/gcc/toplev.c:2076
>> #28 0x0000000000eb715c in toplev::main (this=0x7fffffffdeae, argc=16, argv=0x28a0aa0) at ../../src/gcc/toplev.c:2211
>> #29 0x0000000001bbd183 in main (argc=15, argv=0x7fffffffdfa8) at ../../src/gcc/main.c:39
>>
>> My patch put the apply_location_cache immediately before the read in
>> add_type_duplicate, but maybe there's a better place?
>
> Ugh, quite a maze ;) So your original patch is ok unless Honza comes
> up with something better.
>
> Thanks,
> Richard.
>
>> Thanks
>> Dave
>>
>>>
>>> Richard.
>>>
>>> > Thanks
>>> > Dave
>>> >
>>> > >
>>> > > > gcc/ChangeLog:
>>> > > > PR lto/83121
>>> > > > * ipa-devirt.c (add_type_duplicate): When comparing
>>> > > > memory
>>> > > > layout,
>>> > > > call the lto_location_cache before reading the
>>> > > > DECL_SOURCE_LOCATION of the types.
>>> > > >
>>> > > > gcc/testsuite/ChangeLog:
>>> > > > PR lto/83121
>>> > > > * g++.dg/lto/pr83121_0.C: New test case.
>>> > > > * g++.dg/lto/pr83121_1.C: New test case.
I've noticed that the new tests fail on aarch64/arm:
FAIL: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_0.C line 8)
FAIL: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line 2)
FAIL: g++.dg/lto/pr83121 (test for LTO warnings, pr83121_1.C line 3)
on aarch64, the logs contain:
/gcc/testsuite/g++.dg/lto/pr83121_0.C:6:8: warning: type 'struct
Environment' violates the C++ One Definition Rule [-Wodr]
/gcc/testsuite/g++.dg/lto/pr83121_1.C:1:8: note: a type with different
size is defined in another translation unit
on arm, the logs contain:
/gcc/testsuite/g++.dg/lto/pr83121_0.C:7:10: warning: type 'struct
AsyncHooks' violates the C++ One Definition Rule [-Wodr]
/gcc/testsuite/g++.dg/lto/pr83121_1.C:2:10: note: a different type is
defined in another translation unit
/gcc/testsuite/g++.dg/lto/pr83121_0.C:8:21: note: the first difference
of corresponding definitions is field 'providers_'
/gcc/testsuite/g++.dg/lto/pr83121_1.C:3:21: note: a field of same name
but different type is defined in another translation unit
/gcc/testsuite/g++.dg/lto/pr83121_0.C:7:10: note: array types have
different bounds
/gcc/testsuite/g++.dg/lto/pr83121_0.C:6:8: warning: type 'struct
Environment' violates the C++ One Definition Rule [-Wodr]
/gcc/testsuite/g++.dg/lto/pr83121_1.C:1:8: note: a different type is
defined in another translation unit
/gcc/testsuite/g++.dg/lto/pr83121_0.C:10:14: note: the first
difference of corresponding definitions is field 'async_hooks_'
/gcc/testsuite/g++.dg/lto/pr83121_1.C:5:14: note: a field of same name
but different type is defined in another translation unit
/gcc/testsuite/g++.dg/lto/pr83121_0.C:7:10: note: type 'struct
AsyncHooks' itself violates the C++ One Definition Rule
/gcc/testsuite/g++.dg/lto/pr83121_1.C:2:10: note: the incompatible
type is defined here
and on arm only:
FAIL: g++.dg/lto/pr83121 cp_lto_pr83121_0.o-cp_lto_pr83121_1.o
link, -O0 -flto
Christophe
>>> > > > * lib/lto.exp (lto_handle_diagnostics_for_file): New
>>> > > > procedure,
>>> > > > adapted from DejaGnu's dg-test.
>>> > > > (lto_handle_diagnostics): New procedure.
>>> > > > (lto_prune_warns): Ignore informational notes.
>>> > > > (lto-link-and-maybe-run): Add "messages_by_file" param.
>>> > > > Call lto_handle_diagnostics. Avoid issuing
>>> > > > "unresolved"
>>> > > > for
>>> > > > "execute" when "link" fails if "execute" was not
>>> > > > specified.
>>> > > > (lto-can-handle-directive): New procedure.
>>> > > > (lto-get-options-main): Call lto-can-handle-
>>> > > > directive. Add
>>> > > > a
>>> > > > dg-messages local, using it to set the caller's
>>> > > > dg-messages-by-file for the given source file.
>>> > > > (lto-get-options): Likewise.
>>> > > > (lto-execute): Add dg-messages-by-file local, and pass
>>> > > > it
>>> > > > to
>>> > > > lto-link-and-maybe-run.
>>> > > > ---
>>> > > > gcc/ipa-devirt.c | 7 +-
>>> > > > gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++
>>> > > > gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++
>>> > > > gcc/testsuite/lib/lto.exp | 199
>>> > > > ++++++++++++++++++++++++++++++++++-
>>> > > > 4 files changed, 222 insertions(+), 6 deletions(-)
>>> > > > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_0.C
>>> > > > create mode 100644 gcc/testsuite/g++.dg/lto/pr83121_1.C
>>> > > >
>>> > > > diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
>>> > > > index 540f038..f3d2e4a 100644
>>> > > > --- a/gcc/ipa-devirt.c
>>> > > > +++ b/gcc/ipa-devirt.c
>>> > > > @@ -1844,7 +1844,12 @@ add_type_duplicate (odr_type val, tree
>>> > > > type)
>>> > > > }
>>> > > > }
>>> > > >
>>> > > > - /* Next compare memory layout. */
>>> > > > + /* Next compare memory layout.
>>> > > > + The DECL_SOURCE_LOCATIONs in this invocation came from
>>> > > > LTO
>>> > > > streaming.
>>> > > > + We must apply the location cache to ensure that they are
>>> > > > valid
>>> > > > + before we can pass them to odr_types_equivalent_p (PR
>>> > > > lto/83121). */
>>> > > > + if (lto_location_cache::current_cache)
>>> > > > + lto_location_cache::current_cache->apply_location_cache
>>> > > > ();
>>> > > > if (!odr_types_equivalent_p (val->type, type,
>>> > > > !flag_ltrans && !val-
>>> > > > >odr_violated
>>> > > > && !warned,
>>> > > > &warned, &visited,
>>> > > > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C
>>> > > > b/gcc/testsuite/g++.dg/lto/pr83121_0.C
>>> > > > new file mode 100644
>>> > > > index 0000000..ef894c7
>>> > > > --- /dev/null
>>> > > > +++ b/gcc/testsuite/g++.dg/lto/pr83121_0.C
>>> > > > @@ -0,0 +1,12 @@
>>> > > > +// { dg-lto-do link }
>>> > > > +// { dg-lto-options {{-O0 -flto}} }
>>> > > > +/* We need -O0 to avoid the "Environment" locals in the test
>>> > > > functions
>>> > > > + from being optimized away. */
>>> > > > +
>>> > > > +struct Environment { // { dg-lto-warning "8: type 'struct
>>> > > > Environment' violates the C\\+\\+ One Definition Rule" }
>>> > > > + struct AsyncHooks {
>>> > > > + int providers_[2]; // { dg-lto-message "a field of same
>>> > > > name
>>> > > > but different type is defined in another translation unit" }
>>> > > > + };
>>> > > > + AsyncHooks async_hooks_;
>>> > > > +};
>>> > > > +void fn2() { Environment a; }
>>> > > > diff --git a/gcc/testsuite/g++.dg/lto/pr83121_1.C
>>> > > > b/gcc/testsuite/g++.dg/lto/pr83121_1.C
>>> > > > new file mode 100644
>>> > > > index 0000000..2aef1b5
>>> > > > --- /dev/null
>>> > > > +++ b/gcc/testsuite/g++.dg/lto/pr83121_1.C
>>> > > > @@ -0,0 +1,10 @@
>>> > > > +struct Environment {
>>> > > > + struct AsyncHooks { // { dg-lto-warning "10: type 'struct
>>> > > > AsyncHooks' violates the C\\+\\+ One Definition Rule" }
>>> > > > + int providers_[1]; // { dg-lto-message "the first
>>> > > > difference
>>> > > > of corresponding definitions is field 'providers_'" }
>>> > > > + };
>>> > > > + AsyncHooks async_hooks_;
>>> > > > +};
>>> > > > +void fn1() { Environment a; }
>>> > > > +int main ()
>>> > > > +{
>>> > > > +}
>>> > > > diff --git a/gcc/testsuite/lib/lto.exp
>>> > > > b/gcc/testsuite/lib/lto.exp
>>> > > > index 477f683..e69d8d4 100644
>>> > > > --- a/gcc/testsuite/lib/lto.exp
>>> > > > +++ b/gcc/testsuite/lib/lto.exp
>>> > > > @@ -16,6 +16,122 @@
>>> > > >
>>> > > > # Contributed by Diego Novillo <dnovillo@google.com>
>>> > > >
>>> > > > +# A subroutine of lto_handle_diagnostics: check TEXT for the
>>> > > > expected
>>> > > > +# diagnostics for one specific source file, issuing PASS/FAIL
>>> > > > results.
>>> > > > +# Return TEXT, stripped of any diagnostics that were handled.
>>> > > > +#
>>> > > > +# NAME is the testcase name to use when reporting PASS/FAIL
>>> > > > results.
>>> > > > +# FILENAME is the name (with full path) of the file we're
>>> > > > interested in.
>>> > > > +# MESSAGES_FOR_FILE is a list of expected messages, akin to
>>> > > > DejaGnu's
>>> > > > +# "dg-messages" variable.
>>> > > > +# TEXT is the textual output from the LTO link.
>>> > > > +
>>> > > > +proc lto_handle_diagnostics_for_file { name filename
>>> > > > messages_for_file text } {
>>> > > > + global dg-linenum-format
>>> > > > +
>>> > > > + set filename_without_path [file tail $filename]
>>> > > > +
>>> > > > + # This loop is adapted from the related part of DejaGnu's
>>> > > > dg-
>>> > > > test,
>>> > > > + # with changes as detailed below to cope with the LTO
>>> > > > case.
>>> > > > +
>>> > > > + foreach i ${messages_for_file} {
>>> > > > + verbose "Scanning for message: $i" 4
>>> > > > +
>>> > > > + # Remove all error messages for the line [lindex $i 0]
>>> > > > + # in the source file. If we find any, success!
>>> > > > + set line [lindex $i 0]
>>> > > > + set pattern [lindex $i 2]
>>> > > > + set comment [lindex $i 3]
>>> > > > + verbose "line: $line" 4
>>> > > > + verbose "pattern: $pattern" 4
>>> > > > + verbose "comment: $comment" 4
>>> > > > + #send_user "Before:\n$text\n"
>>> > > > +
>>> > > > + # Unlike dg-test, we use $filename_without_path in this
>>> > > > pattern.
>>> > > > + # This is to ensure that we have the correct file/line
>>> > > > combination.
>>> > > > + # This imposes the restriction that the filename can't
>>> > > > contain
>>> > > > + # any regexp control characters. We have to strip the
>>> > > > path, since
>>> > > > + # e.g. the '+' in "g++.dg" wouldn't be valid.
>>> > > > + set pat
>>> > > > "(^|\n)(\[^\n\]+$filename_without_path$line\[^\n\]*($pattern)\[
>>> > > > ^\n\
>>> > > > ]*\n?)+"
>>> > > > + if {[regsub -all $pat $text "\n" text]} {
>>> > > > + set text [string trimleft $text]
>>> > > > + set ok pass
>>> > > > + set uhoh fail
>>> > > > + } else {
>>> > > > + set ok fail
>>> > > > + set uhoh pass
>>> > > > + }
>>> > > > + #send_user "After:\n$text\n"
>>> > > > +
>>> > > > + # $line will either be a formatted line number or a
>>> > > > number
>>> > > > all by
>>> > > > + # itself. Delete the formatting.
>>> > > > + scan $line ${dg-linenum-format} line
>>> > > > +
>>> > > > + # Unlike dg-test, add the filename to the PASS/FAIL
>>> > > > message
>>> > > > (rather
>>> > > > + # than just the line number) so that the user can
>>> > > > identify
>>> > > > the
>>> > > > + # pertinent directive.
>>> > > > + set describe_where "$filename_without_path line $line"
>>> > > > +
>>> > > > + # Issue the PASS/FAIL, adding "LTO" to the messages
>>> > > > (e.g.
>>> > > > "LTO errors")
>>> > > > + # to distinguish them from the non-LTO case (in case we
>>> > > > ever need to
>>> > > > + # support both).
>>> > > > + switch [lindex $i 1] {
>>> > > > + "ERROR" {
>>> > > > + $ok "$name $comment (test for LTO errors,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "XERROR" {
>>> > > > + x$ok "$name $comment (test for LTO errors,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "WARNING" {
>>> > > > + $ok "$name $comment (test for LTO warnings,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "XWARNING" {
>>> > > > + x$ok "$name $comment (test for LTO warnings,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "BOGUS" {
>>> > > > + $uhoh "$name $comment (test for LTO bogus
>>> > > > messages,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "XBOGUS" {
>>> > > > + x$uhoh "$name $comment (test for LTO bogus
>>> > > > messages, $describe_where)"
>>> > > > + }
>>> > > > + "BUILD" {
>>> > > > + $uhoh "$name $comment (test for LTO build
>>> > > > failure,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "XBUILD" {
>>> > > > + x$uhoh "$name $comment (test for LTO build
>>> > > > failure,
>>> > > > $describe_where)"
>>> > > > + }
>>> > > > + "EXEC" { }
>>> > > > + "XEXEC" { }
>>> > > > + }
>>> > > > + }
>>> > > > + return $text
>>> > > > +}
>>> > > > +
>>> > > > +# Support for checking for link-time diagnostics: check for
>>> > > > +# the expected diagnostics within TEXT, issuing PASS/FAIL
>>> > > > results.
>>> > > > +# Return TEXT, stripped of any diagnostics that were handled.
>>> > > > +#
>>> > > > +# MESSAGES_BY_FILE is a dict; the keys are source files (with
>>> > > > paths)
>>> > > > +# the values are lists of expected messages, akin to DejaGnu's
>>> > > > "dg-messages"
>>> > > > +# variable.
>>> > > > +# TEXT is the textual output from the LTO link.
>>> > > > +
>>> > > > +proc lto_handle_diagnostics { messages_by_file text } {
>>> > > > + global testcase
>>> > > > +
>>> > > > + verbose "lto_handle_diagnostics: entry: $text" 2
>>> > > > + verbose " messages_by_file $messages_by_file" 3
>>> > > > +
>>> > > > + dict for {src dg-messages} $messages_by_file {
>>> > > > + set text [lto_handle_diagnostics_for_file $testcase
>>> > > > $src \
>>> > > > + ${dg-messages} $text]
>>> > > > + }
>>> > > > +
>>> > > > + verbose "lto_handle_diagnostics: exit: $text" 2
>>> > > > +
>>> > > > + return $text
>>> > > > +}
>>> > > > +
>>> > > > # Prune messages that aren't useful.
>>> > > >
>>> > > > proc lto_prune_warns { text } {
>>> > > > @@ -39,6 +155,9 @@ proc lto_prune_warns { text } {
>>> > > > regsub -all "(^|\n)\[ \t\]*\[\(\]file \[^\n\]*
>>> > > > value=\[^\n\]*;
>>> > > > file \[^\n\]* value=\[^\n\]*\[)\];" $text "" text
>>> > > > regsub -all "(^|\n)\[ \t\]*\[^\n\]* definition taken"
>>> > > > $text ""
>>> > > > text
>>> > > >
>>> > > > + # Ignore informational notes.
>>> > > > + regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
>>> > > > +
>>> > > > verbose "lto_prune_warns: exit: $text" 2
>>> > > >
>>> > > > return $text
>>> > > > @@ -175,12 +294,17 @@ proc lto-obj { source dest optall optfile
>>> > > > optstr xfaildata } {
>>> > > > # OPTALL is a list of compiler and linker options to use for
>>> > > > all
>>> > > > tests
>>> > > > # OPTFILE is a list of compiler and linker options to use for
>>> > > > this
>>> > > > test
>>> > > > # OPTSTR is the list of options to list in messages
>>> > > > -proc lto-link-and-maybe-run { testname objlist dest optall
>>> > > > optfile
>>> > > > optstr } {
>>> > > > +# MESSAGES_BY_FILE is a dict of (src, dg-messages)
>>> > > > +proc lto-link-and-maybe-run { testname objlist dest optall
>>> > > > optfile
>>> > > > optstr \
>>> > > > + messages_by_file } {
>>> > > > global testcase
>>> > > > global tool
>>> > > > global compile_type
>>> > > > global board_info
>>> > > >
>>> > > > + verbose "lto-link-and-maybe-run" 2
>>> > > > + verbose " messages_by_file $messages_by_file" 3
>>> > > > +
>>> > > > # Check that all of the objects were built successfully.
>>> > > > foreach obj [split $objlist] {
>>> > > > if ![file_on_host exists $obj] then {
>>> > > > @@ -217,12 +341,17 @@ proc lto-link-and-maybe-run { testname
>>> > > > objlist dest optall optfile optstr } {
>>> > > > set board_info($target_board,ldscript) $saved_ldscript
>>> > > > }
>>> > > >
>>> > > > + # Check for diagnostics specified by directives
>>> > > > + set comp_output [lto_handle_diagnostics $messages_by_file
>>> > > > $comp_output]
>>> > > > +
>>> > > > # Prune unimportant visibility warnings before checking
>>> > > > output.
>>> > > > set comp_output [lto_prune_warns $comp_output]
>>> > > >
>>> > > > if ![${tool}_check_compile "$testcase $testname link"
>>> > > > $optstr
>>> > > > \
>>> > > > $dest $comp_output] then {
>>> > > > - unresolved "$testcase $testname execute $optstr"
>>> > > > + if { ![string compare "execute" $compile_type] } {
>>> > > > + unresolved "$testcase $testname execute $optstr"
>>> > > > + }
>>> > > > return
>>> > > > }
>>> > > >
>>> > > > @@ -243,6 +372,51 @@ proc lto-link-and-maybe-run { testname
>>> > > > objlist
>>> > > > dest optall optfile optstr } {
>>> > > > $status "$testcase $testname execute $optstr"
>>> > > > }
>>> > > >
>>> > > > +# Potentially handle the given dg- directive (a list)
>>> > > > +# Return true is the directive was handled, false otherwise.
>>> > > > +
>>> > > > +proc lto-can-handle-directive { op } {
>>> > > > + set cmd [lindex $op 0]
>>> > > > +
>>> > > > + # dg-warning and dg-message append to dg-messages.
>>> > > > + upvar dg-messages dg-messages
>>> > > > +
>>> > > > + # A list of directives to recognize, and a list of
>>> > > > directives
>>> > > > + # to remap them to.
>>> > > > + # For example, "dg-lto-warning" is implemented by calling
>>> > > > "dg-
>>> > > > warning".
>>> > > > + set directives { dg-lto-warning dg-lto-message }
>>> > > > + set remapped_directives { dg-warning dg-message }
>>> > > > +
>>> > > > + set idx [lsearch -exact $directives $cmd]
>>> > > > + if { $idx != -1 } {
>>> > > > + verbose "remapping from: $op" 4
>>> > > > +
>>> > > > + set remapped_cmd [lindex $remapped_directives $idx]
>>> > > > + set op [lreplace $op 0 0 $remapped_cmd]
>>> > > > +
>>> > > > + verbose "remapped to: $op" 4
>>> > > > +
>>> > > > + set status [catch "$op" errmsg]
>>> > > > + if { $status != 0 } {
>>> > > > + if { 0 && [info exists errorInfo] } {
>>> > > > + # This also prints a backtrace which will just
>>> > > > confuse
>>> > > > + # testcase writers, so it's disabled.
>>> > > > + perror "$name: $errorInfo\n"
>>> > > > + } else {
>>> > > > + perror "$name: $errmsg for \"$op\"\n"
>>> > > > + }
>>> > > > + # ??? The call to unresolved here is necessary to
>>> > > > clear
>>> > > > `errcnt'.
>>> > > > + # What we really need is a proc like perror that
>>> > > > doesn't set errcnt.
>>> > > > + # It should also set exit_status to 1.
>>> > > > + unresolved "$name: $errmsg for \"$op\""
>>> > > > + }
>>> > > > +
>>> > > > + return true
>>> > > > + }
>>> > > > +
>>> > > > + return false
>>> > > > +}
>>> > > > +
>>> > > > # lto-get-options-main -- get target requirements for a test
>>> > > > and
>>> > > > # options for the primary source file and the test as a whole
>>> > > > #
>>> > > > @@ -266,6 +440,10 @@ proc lto-get-options-main { src } {
>>> > > > upvar dg-final-code dg-final-code
>>> > > > set dg-final-code ""
>>> > > >
>>> > > > + # dg-warning and dg-message append to dg-messages.
>>> > > > + upvar dg-messages-by-file dg-messages-by-file
>>> > > > + set dg-messages ""
>>> > > > +
>>> > > > set tmp [dg-get-options $src]
>>> > > > verbose "getting options for $src: $tmp"
>>> > > > foreach op $tmp {
>>> > > > @@ -342,12 +520,15 @@ proc lto-get-options-main { src } {
>>> > > > } else {
>>> > > > append dg-final-code "[lindex $op 2]\n"
>>> > > > }
>>> > > > - } else {
>>> > > > + } elseif { ![lto-can-handle-directive $op] } {
>>> > > > # Ignore unrecognized dg- commands, but warn about
>>> > > > them.
>>> > > > warning "lto.exp does not support $cmd"
>>> > > > }
>>> > > > }
>>> > > >
>>> > > > + verbose "dg-messages: ${dg-messages}" 3
>>> > > > + dict append dg-messages-by-file $src ${dg-messages}
>>> > > > +
>>> > > > # Return flags to use for compiling the primary source
>>> > > > file
>>> > > > and for
>>> > > > # linking.
>>> > > > verbose "dg-extra-tool-flags for main is ${dg-extra-tool-
>>> > > > flags}"
>>> > > > @@ -373,6 +554,10 @@ proc lto-get-options { src } {
>>> > > > # dg-xfail-if needs access to dg-do-what.
>>> > > > upvar dg-do-what dg-do-what
>>> > > >
>>> > > > + # dg-warning appends to dg-messages.
>>> > > > + upvar dg-messages-by-file dg-messages-by-file
>>> > > > + set dg-messages ""
>>> > > > +
>>> > > > set tmp [dg-get-options $src]
>>> > > > foreach op $tmp {
>>> > > > set cmd [lindex $op 0]
>>> > > > @@ -386,12 +571,15 @@ proc lto-get-options { src } {
>>> > > > }
>>> > > > } elseif { [string match "dg-require-*" $cmd] } {
>>> > > > warning "lto.exp does not support $cmd in secondary
>>> > > > source files"
>>> > > > - } else {
>>> > > > + } elseif { ![lto-can-handle-directive $op] } {
>>> > > > # Ignore unrecognized dg- commands, but warn about
>>> > > > them.
>>> > > > warning "lto.exp does not support $cmd in secondary
>>> > > > source files"
>>> > > > }
>>> > > > }
>>> > > >
>>> > > > + verbose "dg-messages: ${dg-messages}" 3
>>> > > > + dict append dg-messages-by-file $src ${dg-messages}
>>> > > > +
>>> > > > return ${dg-extra-tool-flags}
>>> > > > }
>>> > > >
>>> > > > @@ -421,6 +609,7 @@ proc lto-execute { src1 sid } {
>>> > > > verbose "lto-execute: $src1" 1
>>> > > > set compile_type "run"
>>> > > > set dg-do-what [list ${dg-do-what-default} "" P]
>>> > > > + set dg-messages-by-file [dict create]
>>> > > > set extra_flags(0) [lto-get-options-main $src1]
>>> > > > set compile_xfail(0) ""
>>> > > >
>>> > > > @@ -544,7 +733,7 @@ proc lto-execute { src1 sid } {
>>> > > > lto-link-and-maybe-run \
>>> > > > "[lindex $obj_list 0]-[lindex $obj_list
>>> > > > end]" \
>>> > > > $obj_list $execname $filtered ${dg-extra-
>>> > > > ld-
>>> > > > options} \
>>> > > > - $filtered
>>> > > > + $filtered ${dg-messages-by-file}
>>> > > > }
>>> > > >
>>> > > >
>>> > > > --
>>> > > > 1.8.5.3
>>> > > >
>>>
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-18 10:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-04 21:49 [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121) David Malcolm
2018-01-05 9:36 ` Richard Biener
2018-01-05 22:55 ` David Malcolm
2018-01-06 7:44 ` Richard Biener
2018-01-08 19:38 ` David Malcolm
2018-01-15 10:09 ` Richard Biener
2018-01-18 12:41 ` Christophe Lyon
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).