From 0a425a4ac0c3c2ad22086c490efccbd9a0849df3 Mon Sep 17 00:00:00 2001 From: dmalcolm Date: Wed, 17 Jan 2018 16:56:56 +0000 Subject: [PATCH] lto, testsuite: Fix ICE in -Wodr (PR lto/83121) 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. 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". 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. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@256801 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/ChangeLog | 7 ++ gcc/ipa-devirt.c | 7 +- gcc/testsuite/ChangeLog | 20 ++++ gcc/testsuite/g++.dg/lto/pr83121_0.C | 12 +++ gcc/testsuite/g++.dg/lto/pr83121_1.C | 10 ++ gcc/testsuite/lib/lto.exp | 199 ++++++++++++++++++++++++++++++++++- 6 files changed, 249 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/ChangeLog b/gcc/ChangeLog index 85262662b49..7b0146b7e52 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2018-01-17 David Malcolm + + 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. + 2018-01-17 Wilco Dijkstra Richard Sandiford diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c index cc551d636fd..f66dc45deb8 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/ChangeLog b/gcc/testsuite/ChangeLog index f51f41710c6..dd25a3746b8 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,23 @@ +2018-01-17 David Malcolm + + 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. + 2018-01-17 Wilco Dijkstra Richard Sandiford 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 00000000000..ef894c7c868 --- /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 00000000000..2aef1b50957 --- /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 8cfcecae247..11d113ce675 100644 --- a/gcc/testsuite/lib/lto.exp +++ b/gcc/testsuite/lib/lto.exp @@ -16,6 +16,122 @@ # Contributed by Diego Novillo +# 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} } -- 2.11.4.GIT