On January 5, 2018 11:55:11 PM GMT+01:00, David Malcolm <dmalc...@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 <dmalc...@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 <dnovi...@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 >> >