On January 5, 2018 11:55:11 PM GMT+01:00, David Malcolm <[email protected]>
wrote:
>On Fri, 2018-01-05 at 10:36 +0100, Richard Biener wrote:
>> On Thu, Jan 4, 2018 at 10:52 PM, David Malcolm <[email protected]>
>> 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 <[email protected]>
>> >
>> > +# 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
>> >
