lto, testsuite: Fix ICE in -Wodr (PR lto/83121)
authorDavid Malcolm <dmalcolm@redhat.com>
Wed, 17 Jan 2018 16:56:56 +0000 (16:56 +0000)
committerDavid Malcolm <dmalcolm@gcc.gnu.org>
Wed, 17 Jan 2018 16:56:56 +0000 (16:56 +0000)
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.

From-SVN: r256801

gcc/ChangeLog
gcc/ipa-devirt.c
gcc/testsuite/ChangeLog
gcc/testsuite/g++.dg/lto/pr83121_0.C [new file with mode: 0644]
gcc/testsuite/g++.dg/lto/pr83121_1.C [new file with mode: 0644]
gcc/testsuite/lib/lto.exp

index 85262662b49eb8304bbb58f5df35ff3c529e0bef..7b0146b7e52b4a2274986ea9e199eda376ef7982 100644 (file)
@@ -1,3 +1,10 @@
+2018-01-17  David Malcolm  <dmalcolm@redhat.com>
+
+       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  <wdijkstr@arm.com>
            Richard Sandiford  <richard.sandiford@linaro.org>
 
index cc551d636fd7c5e097858af4fc5db8dff99d69b1..f66dc45deb8668372b620d2348c39bef835f8c90 100644 (file)
@@ -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,
index f51f41710c686986d414896e782295581493f16f..dd25a3746b87dd7c2815399b077a9632c950051d 100644 (file)
@@ -1,3 +1,23 @@
+2018-01-17  David Malcolm  <dmalcolm@redhat.com>
+
+       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  <wdijkstr@arm.com>
            Richard Sandiford  <richard.sandiford@linaro.org>
 
diff --git a/gcc/testsuite/g++.dg/lto/pr83121_0.C b/gcc/testsuite/g++.dg/lto/pr83121_0.C
new file mode 100644 (file)
index 0000000..ef894c7
--- /dev/null
@@ -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 (file)
index 0000000..2aef1b5
--- /dev/null
@@ -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 ()
+{
+}
index 8cfcecae247a3fa794d3d24a2152a7e4defd8787..11d113ce6754f6911bda782778b337d64075ff9c 100644 (file)
 
 # Contributed by Diego Novillo <dnovillo@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}
        }