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