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?
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
> >
