* Improve LTO type checking during symtab merging
@ 2015-04-27 8:34 Jan Hubicka
2015-04-27 8:54 ` Richard Biener
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jan Hubicka @ 2015-04-27 8:34 UTC (permalink / raw)
To: gcc-patches, rguenther
Hi,
this patch strengten ODR checking for requiring match on declarations
(if both of them are defined in C++ tranlsation unit). Currently
ipa-symtab.c will warn only if declarations are incompatible in
useless_type_conversion sense. Now we warn more often, i.e. in the following
example:
t.C:
char a;
t2.C:
#include <stdtype.h>
extern int8_t a;
void *b=&a;
Will lead to warning:
t2.C:2:15: warning: type of �a� does not match original declaration
extern int8_t a;
^
<built-in>: note: type name �char� should match type name �signed char�
/usr/include/stdint.h:37:22: note: the incompatible type is defined here
typedef signed char int8_t;
^
/aux/hubicka/t.C:1:6: note: previously declared here
char a;
^
Funnilly enough we do not get warning when t2.C is just "signed char"
or "unsigned char" because that is builtin type and thus non-ODR. Something
I need to deal with incrementally.
I also added call to warn_types_mismatch that outputs extra note about
what is wrong with the type: in C++ the mismatch is often carried over
several levels on declarations and it is hard to work out the reason
without extra info.
Richard, According to comments, it seems that the code was tailored to not
output warning when we can avoid wrong code issues on mismatches based on fact
that linker would be also silent. I am particularly entertained by the
following hunk which I killed:
- if (!types_compatible_p (TREE_TYPE (prevailing_decl),
- TREE_TYPE (decl)))
- /* If we don't have a merged type yet...sigh. The linker
- wouldn't complain if the types were mismatched, so we
- probably shouldn't either. Just use the type from
- whichever decl appears to be associated with the
- definition. If for some odd reason neither decl is, the
- older one wins. */
- (void) 0;
It is fun we go through the trouble of comparing types and then do nothing ;)
Type merging is also completed at this time, so at least the comment looks
out of date.
I think as QOI issue, we do want to warn in cases the code is inconsistent (it
is pretty much surely a bug), but perhaps we may want to have the warnings with
-Wodr only and use slightly different warning and different -W switch for
warnings that unavoidably leads to wrong code surprises? This should be easy
to implement by adding two levels into new warn_type_compatibility_p predicate.
I am personaly also OK however with warning always or making all the warnings
-Wodr.
What do you think?
Incrementally I am heading towards proper definition of decl compatibility that
I can plug into symtab merging and avoid merging incompatible decls (so
FORTIFY_SOURCE works).
lto-symtab and ipa-icf both have some knowledge of the problem, I want to get
both right and factor out common logic.
Other improvement is to preserve the ODR type info when non-ODR variant
previals so one can diagnose clash in between C++ units even in mixed
language linktimes.
Bootstrapped/regtested x86_64-linux with no new ODR warnings hit by -Werror.
Honza
* ipa-devirt.c (register_odr_type): Be ready for non-odr main variant
of odr type.
* ipa-utils.h (warn_types_mismatch): New.
* lto/lto-symtab.c (warn_type_compatibility_p): Break out from ...
(lto_symtab_merge): ... here.
(lto_symtab_merge_decls_2): Improve warnings.
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c (revision 222391)
+++ ipa-devirt.c (working copy)
@@ -2029,10 +2030,14 @@ register_odr_type (tree type)
if (in_lto_p)
odr_vtable_hash = new odr_vtable_hash_type (23);
}
- /* Arrange things to be nicer and insert main variants first. */
- if (odr_type_p (TYPE_MAIN_VARIANT (type)))
+ /* Arrange things to be nicer and insert main variants first.
+ ??? fundamental prerecorded types do not have mangled names; this
+ makes it possible that non-ODR type is main_odr_variant of ODR type.
+ Things may get smoother if LTO FE set mangled name of those types same
+ way as C++ FE does. */
+ if (odr_type_p (main_odr_variant (TYPE_MAIN_VARIANT (type))))
get_odr_type (TYPE_MAIN_VARIANT (type), true);
- if (TYPE_MAIN_VARIANT (type) != type)
+ if (TYPE_MAIN_VARIANT (type) != type && odr_type_p (main_odr_variant (type)))
get_odr_type (type, true);
}
Index: ipa-utils.h
===================================================================
--- ipa-utils.h (revision 222391)
+++ ipa-utils.h (working copy)
@@ -84,6 +84,7 @@ bool types_must_be_same_for_odr (tree, t
bool types_odr_comparable (tree, tree, bool strict = false);
cgraph_node *try_speculative_devirtualization (tree, HOST_WIDE_INT,
ipa_polymorphic_call_context);
+void warn_types_mismatch (tree t1, tree t2);
/* Return vector containing possible targets of polymorphic call E.
If COMPLETEP is non-NULL, store true if the list is complete.
Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c (revision 222391)
+++ lto/lto-symtab.c (working copy)
@@ -203,45 +203,16 @@ lto_varpool_replace_node (varpool_node *
vnode->remove ();
}
-/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
- Return false if the symbols are not fully compatible and a diagnostic
- should be emitted. */
+/* Return true if we want to output waring about T1 and T2. */
static bool
-lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
+warn_type_compatibility_p (tree prevailing_type, tree type)
{
- tree prevailing_decl = prevailing->decl;
- tree decl = entry->decl;
- tree prevailing_type, type;
-
- if (prevailing_decl == decl)
+ /* C++ provide robust way to check for type compatibility via the ODR
+ rule. */
+ if (odr_type_p (prevailing_type) && odr_type_p (type)
+ && !types_same_for_odr (prevailing_type, type, true))
return true;
-
- /* Merge decl state in both directions, we may still end up using
- the new decl. */
- TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
- TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
-
- /* The linker may ask us to combine two incompatible symbols.
- Detect this case and notify the caller of required diagnostics. */
-
- if (TREE_CODE (decl) == FUNCTION_DECL)
- {
- if (!types_compatible_p (TREE_TYPE (prevailing_decl),
- TREE_TYPE (decl)))
- /* If we don't have a merged type yet...sigh. The linker
- wouldn't complain if the types were mismatched, so we
- probably shouldn't either. Just use the type from
- whichever decl appears to be associated with the
- definition. If for some odd reason neither decl is, the
- older one wins. */
- (void) 0;
-
- return true;
- }
-
- /* Now we exclusively deal with VAR_DECLs. */
-
/* Sharing a global symbol is a strong hint that two types are
compatible. We could use this information to complete
incomplete pointed-to types more aggressively here, ignoring
@@ -254,19 +225,22 @@ lto_symtab_merge (symtab_node *prevailin
??? In principle we might want to only warn for structurally
incompatible types here, but unless we have protective measures
for TBAA in place that would hide useful information. */
- prevailing_type = TYPE_MAIN_VARIANT (TREE_TYPE (prevailing_decl));
- type = TYPE_MAIN_VARIANT (TREE_TYPE (decl));
+ prevailing_type = TYPE_MAIN_VARIANT (prevailing_type);
+ type = TYPE_MAIN_VARIANT (type);
if (!types_compatible_p (prevailing_type, type))
{
+ if (TREE_CODE (prevailing_type) == FUNCTION_TYPE
+ || TREE_CODE (type) == METHOD_TYPE)
+ return true;
if (COMPLETE_TYPE_P (type))
- return false;
+ return true;
/* If type is incomplete then avoid warnings in the cases
that TBAA handles just fine. */
if (TREE_CODE (prevailing_type) != TREE_CODE (type))
- return false;
+ return true;
if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
{
@@ -280,10 +254,10 @@ lto_symtab_merge (symtab_node *prevailin
}
if (TREE_CODE (tem1) != TREE_CODE (tem2))
- return false;
+ return true;
if (!types_compatible_p (tem1, tem2))
- return false;
+ return true;
}
/* Fallthru. Compatible enough. */
@@ -292,6 +266,43 @@ lto_symtab_merge (symtab_node *prevailin
/* ??? We might want to emit a warning here if type qualification
differences were spotted. Do not do this unconditionally though. */
+ return false;
+}
+
+/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
+ Return false if the symbols are not fully compatible and a diagnostic
+ should be emitted. */
+
+static bool
+lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
+{
+ tree prevailing_decl = prevailing->decl;
+ tree decl = entry->decl;
+
+ if (prevailing_decl == decl)
+ return true;
+
+ /* Merge decl state in both directions, we may still end up using
+ the new decl. */
+ TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
+ TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
+
+ /* The linker may ask us to combine two incompatible symbols.
+ Detect this case and notify the caller of required diagnostics. */
+
+ if (TREE_CODE (decl) == FUNCTION_DECL)
+ {
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
+ TREE_TYPE (decl)))
+ return false;
+
+ return true;
+ }
+
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
+ TREE_TYPE (decl)))
+ return false;
+
/* There is no point in comparing too many details of the decls here.
The type compatibility checks or the completing of types has properly
dealt with most issues. */
@@ -483,12 +494,18 @@ lto_symtab_merge_decls_2 (symtab_node *f
/* Diagnose all mismatched re-declarations. */
FOR_EACH_VEC_ELT (mismatches, i, decl)
{
- if (!types_compatible_p (TREE_TYPE (prevailing->decl),
- TREE_TYPE (decl)))
- diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
- "type of %qD does not match original "
- "declaration", decl);
-
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
+ TREE_TYPE (decl)))
+ {
+ bool diag;
+ diag = warning_at (DECL_SOURCE_LOCATION (decl), 0,
+ "type of %qD does not match original "
+ "declaration", decl);
+ diagnosed_p |= diag;
+ if (diag)
+ warn_types_mismatch (TREE_TYPE (prevailing->decl),
+ TREE_TYPE (decl));
+ }
else if ((DECL_USER_ALIGN (prevailing->decl)
&& DECL_USER_ALIGN (decl))
&& DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-04-27 8:34 Improve LTO type checking during symtab merging Jan Hubicka
@ 2015-04-27 8:54 ` Richard Biener
2015-04-28 14:42 ` Jan Hubicka
2015-04-28 2:53 ` Jan Hubicka
2015-04-28 4:07 ` Jan Hubicka
2 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2015-04-27 8:54 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches
On Mon, 27 Apr 2015, Jan Hubicka wrote:
> Hi,
> this patch strengten ODR checking for requiring match on declarations
> (if both of them are defined in C++ tranlsation unit). Currently
> ipa-symtab.c will warn only if declarations are incompatible in
> useless_type_conversion sense. Now we warn more often, i.e. in the following
> example:
>
> t.C:
> char a;
>
> t2.C:
> #include <stdtype.h>
> extern int8_t a;
> void *b=&a;
>
> Will lead to warning:
> t2.C:2:15: warning: type of ???a??? does not match original declaration
> extern int8_t a;
> ^
> <built-in>: note: type name ???char??? should match type name ???signed char???
> /usr/include/stdint.h:37:22: note: the incompatible type is defined here
> typedef signed char int8_t;
> ^
> /aux/hubicka/t.C:1:6: note: previously declared here
> char a;
> ^
>
> Funnilly enough we do not get warning when t2.C is just "signed char"
> or "unsigned char" because that is builtin type and thus non-ODR. Something
> I need to deal with incrementally.
>
> I also added call to warn_types_mismatch that outputs extra note about
> what is wrong with the type: in C++ the mismatch is often carried over
> several levels on declarations and it is hard to work out the reason
> without extra info.
>
> Richard, According to comments, it seems that the code was tailored to not
> output warning when we can avoid wrong code issues on mismatches based on fact
> that linker would be also silent. I am particularly entertained by the
> following hunk which I killed:
>
> - if (!types_compatible_p (TREE_TYPE (prevailing_decl),
> - TREE_TYPE (decl)))
> - /* If we don't have a merged type yet...sigh. The linker
> - wouldn't complain if the types were mismatched, so we
> - probably shouldn't either. Just use the type from
> - whichever decl appears to be associated with the
> - definition. If for some odd reason neither decl is, the
> - older one wins. */
> - (void) 0;
>
> It is fun we go through the trouble of comparing types and then do nothing ;)
> Type merging is also completed at this time, so at least the comment looks
> out of date.
>
> I think as QOI issue, we do want to warn in cases the code is
> inconsistent (it is pretty much surely a bug), but perhaps we may want
> to have the warnings with -Wodr only and use slightly different warning
> and different -W switch for warnings that unavoidably leads to wrong
> code surprises? This should be easy to implement by adding two levels
> into new warn_type_compatibility_p predicate. I am personaly also OK
> however with warning always or making all the warnings -Wodr.
>
> What do you think?
Only emitting the warnings with -Wodr looks good to me. I can't see
how we can decide what cases lead to wrong code surprises and what not
(other than using types_compatible_p ...). Wrong-code can only(?) happen
if we happen to inline in a way that makes the incosistency visible in
a single function.
> Incrementally I am heading towards proper definition of decl
> compatibility that I can plug into symtab merging and avoid merging
> incompatible decls (so FORTIFY_SOURCE works).
>
> lto-symtab and ipa-icf both have some knowledge of the problem, I want to get
> both right and factor out common logic.
Sounds good.
> Other improvement is to preserve the ODR type info when non-ODR variant
> previals so one can diagnose clash in between C++ units even in mixed
> language linktimes.
Hmm, but then merging ODR with non-ODR variant is already pointing to
a ODR violation? So why do you need to retain that info?
Btw, there is that old PR41227 which has both wrong-code and diagnostic
impact...
Richard.
> Bootstrapped/regtested x86_64-linux with no new ODR warnings hit by -Werror.
>
> Honza
>
> * ipa-devirt.c (register_odr_type): Be ready for non-odr main variant
> of odr type.
> * ipa-utils.h (warn_types_mismatch): New.
> * lto/lto-symtab.c (warn_type_compatibility_p): Break out from ...
> (lto_symtab_merge): ... here.
> (lto_symtab_merge_decls_2): Improve warnings.
>
> Index: ipa-devirt.c
> ===================================================================
> --- ipa-devirt.c (revision 222391)
> +++ ipa-devirt.c (working copy)
> @@ -2029,10 +2030,14 @@ register_odr_type (tree type)
> if (in_lto_p)
> odr_vtable_hash = new odr_vtable_hash_type (23);
> }
> - /* Arrange things to be nicer and insert main variants first. */
> - if (odr_type_p (TYPE_MAIN_VARIANT (type)))
> + /* Arrange things to be nicer and insert main variants first.
> + ??? fundamental prerecorded types do not have mangled names; this
> + makes it possible that non-ODR type is main_odr_variant of ODR type.
> + Things may get smoother if LTO FE set mangled name of those types same
> + way as C++ FE does. */
> + if (odr_type_p (main_odr_variant (TYPE_MAIN_VARIANT (type))))
> get_odr_type (TYPE_MAIN_VARIANT (type), true);
> - if (TYPE_MAIN_VARIANT (type) != type)
> + if (TYPE_MAIN_VARIANT (type) != type && odr_type_p (main_odr_variant (type)))
> get_odr_type (type, true);
> }
>
> Index: ipa-utils.h
> ===================================================================
> --- ipa-utils.h (revision 222391)
> +++ ipa-utils.h (working copy)
> @@ -84,6 +84,7 @@ bool types_must_be_same_for_odr (tree, t
> bool types_odr_comparable (tree, tree, bool strict = false);
> cgraph_node *try_speculative_devirtualization (tree, HOST_WIDE_INT,
> ipa_polymorphic_call_context);
> +void warn_types_mismatch (tree t1, tree t2);
>
> /* Return vector containing possible targets of polymorphic call E.
> If COMPLETEP is non-NULL, store true if the list is complete.
> Index: lto/lto-symtab.c
> ===================================================================
> --- lto/lto-symtab.c (revision 222391)
> +++ lto/lto-symtab.c (working copy)
> @@ -203,45 +203,16 @@ lto_varpool_replace_node (varpool_node *
> vnode->remove ();
> }
>
> -/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
> - Return false if the symbols are not fully compatible and a diagnostic
> - should be emitted. */
> +/* Return true if we want to output waring about T1 and T2. */
>
> static bool
> -lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +warn_type_compatibility_p (tree prevailing_type, tree type)
> {
> - tree prevailing_decl = prevailing->decl;
> - tree decl = entry->decl;
> - tree prevailing_type, type;
> -
> - if (prevailing_decl == decl)
> + /* C++ provide robust way to check for type compatibility via the ODR
> + rule. */
> + if (odr_type_p (prevailing_type) && odr_type_p (type)
> + && !types_same_for_odr (prevailing_type, type, true))
> return true;
> -
> - /* Merge decl state in both directions, we may still end up using
> - the new decl. */
> - TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
> - TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
> -
> - /* The linker may ask us to combine two incompatible symbols.
> - Detect this case and notify the caller of required diagnostics. */
> -
> - if (TREE_CODE (decl) == FUNCTION_DECL)
> - {
> - if (!types_compatible_p (TREE_TYPE (prevailing_decl),
> - TREE_TYPE (decl)))
> - /* If we don't have a merged type yet...sigh. The linker
> - wouldn't complain if the types were mismatched, so we
> - probably shouldn't either. Just use the type from
> - whichever decl appears to be associated with the
> - definition. If for some odd reason neither decl is, the
> - older one wins. */
> - (void) 0;
> -
> - return true;
> - }
> -
> - /* Now we exclusively deal with VAR_DECLs. */
> -
> /* Sharing a global symbol is a strong hint that two types are
> compatible. We could use this information to complete
> incomplete pointed-to types more aggressively here, ignoring
> @@ -254,19 +225,22 @@ lto_symtab_merge (symtab_node *prevailin
> ??? In principle we might want to only warn for structurally
> incompatible types here, but unless we have protective measures
> for TBAA in place that would hide useful information. */
> - prevailing_type = TYPE_MAIN_VARIANT (TREE_TYPE (prevailing_decl));
> - type = TYPE_MAIN_VARIANT (TREE_TYPE (decl));
> + prevailing_type = TYPE_MAIN_VARIANT (prevailing_type);
> + type = TYPE_MAIN_VARIANT (type);
>
> if (!types_compatible_p (prevailing_type, type))
> {
> + if (TREE_CODE (prevailing_type) == FUNCTION_TYPE
> + || TREE_CODE (type) == METHOD_TYPE)
> + return true;
> if (COMPLETE_TYPE_P (type))
> - return false;
> + return true;
>
> /* If type is incomplete then avoid warnings in the cases
> that TBAA handles just fine. */
>
> if (TREE_CODE (prevailing_type) != TREE_CODE (type))
> - return false;
> + return true;
>
> if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
> {
> @@ -280,10 +254,10 @@ lto_symtab_merge (symtab_node *prevailin
> }
>
> if (TREE_CODE (tem1) != TREE_CODE (tem2))
> - return false;
> + return true;
>
> if (!types_compatible_p (tem1, tem2))
> - return false;
> + return true;
> }
>
> /* Fallthru. Compatible enough. */
> @@ -292,6 +266,43 @@ lto_symtab_merge (symtab_node *prevailin
> /* ??? We might want to emit a warning here if type qualification
> differences were spotted. Do not do this unconditionally though. */
>
> + return false;
> +}
> +
> +/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
> + Return false if the symbols are not fully compatible and a diagnostic
> + should be emitted. */
> +
> +static bool
> +lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +{
> + tree prevailing_decl = prevailing->decl;
> + tree decl = entry->decl;
> +
> + if (prevailing_decl == decl)
> + return true;
> +
> + /* Merge decl state in both directions, we may still end up using
> + the new decl. */
> + TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
> + TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
> +
> + /* The linker may ask us to combine two incompatible symbols.
> + Detect this case and notify the caller of required diagnostics. */
> +
> + if (TREE_CODE (decl) == FUNCTION_DECL)
> + {
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> + TREE_TYPE (decl)))
> + return false;
> +
> + return true;
> + }
> +
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> + TREE_TYPE (decl)))
> + return false;
> +
> /* There is no point in comparing too many details of the decls here.
> The type compatibility checks or the completing of types has properly
> dealt with most issues. */
> @@ -483,12 +494,18 @@ lto_symtab_merge_decls_2 (symtab_node *f
> /* Diagnose all mismatched re-declarations. */
> FOR_EACH_VEC_ELT (mismatches, i, decl)
> {
> - if (!types_compatible_p (TREE_TYPE (prevailing->decl),
> - TREE_TYPE (decl)))
> - diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
> - "type of %qD does not match original "
> - "declaration", decl);
> -
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
> + TREE_TYPE (decl)))
> + {
> + bool diag;
> + diag = warning_at (DECL_SOURCE_LOCATION (decl), 0,
> + "type of %qD does not match original "
> + "declaration", decl);
> + diagnosed_p |= diag;
> + if (diag)
> + warn_types_mismatch (TREE_TYPE (prevailing->decl),
> + TREE_TYPE (decl));
> + }
> else if ((DECL_USER_ALIGN (prevailing->decl)
> && DECL_USER_ALIGN (decl))
> && DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Improve LTO type checking during symtab merging
2015-04-27 8:34 Improve LTO type checking during symtab merging Jan Hubicka
2015-04-27 8:54 ` Richard Biener
@ 2015-04-28 2:53 ` Jan Hubicka
2015-04-28 4:07 ` Jan Hubicka
2 siblings, 0 replies; 10+ messages in thread
From: Jan Hubicka @ 2015-04-28 2:53 UTC (permalink / raw)
To: gcc-patches, rguenther
Hi,
this patch strengten ODR checking for requiring match on declarations
(if both of them are defined in C++ tranlsation unit). Currently
ipa-symtab.c will warn only if declarations are incompatible in
useless_type_conversion sense. Now we warn more often, i.e. in the following
example:
t.C:
char a;
t2.C:
#include <stdtype.h>
extern int8_t a;
void *b=&a;
Will lead to warning:
t2.C:2:15: warning: type of �a� does not match original declaration
extern int8_t a;
^
<built-in>: note: type name �char� should match type name �signed char�
/usr/include/stdint.h:37:22: note: the incompatible type is defined here
typedef signed char int8_t;
^
/aux/hubicka/t.C:1:6: note: previously declared here
char a;
^
Funnilly enough we do not get warning when t2.C is just "signed char"
or "unsigned char" because that is builtin type and thus non-ODR. Something
I need to deal with incrementally.
I also added call to warn_types_mismatch that outputs extra note about
what is wrong with the type: in C++ the mismatch is often carried over
several levels on declarations and it is hard to work out the reason
without extra info.
Richard, According to comments, it seems that the code was tailored to not
output warning when we can avoid wrong code issues on mismatches based on fact
that linker would be also silent. I am particularly entertained by the
following hunk which I killed:
- if (!types_compatible_p (TREE_TYPE (prevailing_decl),
- TREE_TYPE (decl)))
- /* If we don't have a merged type yet...sigh. The linker
- wouldn't complain if the types were mismatched, so we
- probably shouldn't either. Just use the type from
- whichever decl appears to be associated with the
- definition. If for some odd reason neither decl is, the
- older one wins. */
- (void) 0;
It is fun we go through the trouble of comparing types and then do nothing ;)
Type merging is also completed at this time, so at least the comment looks
out of date.
I think as QOI issue, we do want to warn in cases the code is inconsistent (it
is pretty much surely a bug), but perhaps we may want to have the warnings with
-Wodr only and use slightly different warning and different -W switch for
warnings that unavoidably leads to wrong code surprises? This should be easy
to implement by adding two levels into new warn_type_compatibility_p predicate.
I am personaly also OK however with warning always or making all the warnings
-Wodr.
What do you think?
Incrementally I am heading towards proper definition of decl compatibility that
I can plug into symtab merging and avoid merging incompatible decls (so
FORTIFY_SOURCE works).
lto-symtab and ipa-icf both have some knowledge of the problem, I want to get
both right and factor out common logic.
Other improvement is to preserve the ODR type info when non-ODR variant
previals so one can diagnose clash in between C++ units even in mixed
language linktimes.
Bootstrapped/regtested x86_64-linux with no new ODR warnings hit by -Werror.
Honza
* ipa-devirt.c (register_odr_type): Be ready for non-odr main variant
of odr type.
* ipa-utils.h (warn_types_mismatch): New.
* lto/lto-symtab.c (warn_type_compatibility_p): Break out from ...
(lto_symtab_merge): ... here.
(lto_symtab_merge_decls_2): Improve warnings.
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c (revision 222391)
+++ ipa-devirt.c (working copy)
@@ -2029,10 +2030,14 @@ register_odr_type (tree type)
if (in_lto_p)
odr_vtable_hash = new odr_vtable_hash_type (23);
}
- /* Arrange things to be nicer and insert main variants first. */
- if (odr_type_p (TYPE_MAIN_VARIANT (type)))
+ /* Arrange things to be nicer and insert main variants first.
+ ??? fundamental prerecorded types do not have mangled names; this
+ makes it possible that non-ODR type is main_odr_variant of ODR type.
+ Things may get smoother if LTO FE set mangled name of those types same
+ way as C++ FE does. */
+ if (odr_type_p (main_odr_variant (TYPE_MAIN_VARIANT (type))))
get_odr_type (TYPE_MAIN_VARIANT (type), true);
- if (TYPE_MAIN_VARIANT (type) != type)
+ if (TYPE_MAIN_VARIANT (type) != type && odr_type_p (main_odr_variant (type)))
get_odr_type (type, true);
}
Index: ipa-utils.h
===================================================================
--- ipa-utils.h (revision 222391)
+++ ipa-utils.h (working copy)
@@ -84,6 +84,7 @@ bool types_must_be_same_for_odr (tree, t
bool types_odr_comparable (tree, tree, bool strict = false);
cgraph_node *try_speculative_devirtualization (tree, HOST_WIDE_INT,
ipa_polymorphic_call_context);
+void warn_types_mismatch (tree t1, tree t2);
/* Return vector containing possible targets of polymorphic call E.
If COMPLETEP is non-NULL, store true if the list is complete.
Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c (revision 222391)
+++ lto/lto-symtab.c (working copy)
@@ -203,45 +203,16 @@ lto_varpool_replace_node (varpool_node *
vnode->remove ();
}
-/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
- Return false if the symbols are not fully compatible and a diagnostic
- should be emitted. */
+/* Return true if we want to output waring about T1 and T2. */
static bool
-lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
+warn_type_compatibility_p (tree prevailing_type, tree type)
{
- tree prevailing_decl = prevailing->decl;
- tree decl = entry->decl;
- tree prevailing_type, type;
-
- if (prevailing_decl == decl)
+ /* C++ provide robust way to check for type compatibility via the ODR
+ rule. */
+ if (odr_type_p (prevailing_type) && odr_type_p (type)
+ && !types_same_for_odr (prevailing_type, type, true))
return true;
-
- /* Merge decl state in both directions, we may still end up using
- the new decl. */
- TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
- TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
-
- /* The linker may ask us to combine two incompatible symbols.
- Detect this case and notify the caller of required diagnostics. */
-
- if (TREE_CODE (decl) == FUNCTION_DECL)
- {
- if (!types_compatible_p (TREE_TYPE (prevailing_decl),
- TREE_TYPE (decl)))
- /* If we don't have a merged type yet...sigh. The linker
- wouldn't complain if the types were mismatched, so we
- probably shouldn't either. Just use the type from
- whichever decl appears to be associated with the
- definition. If for some odd reason neither decl is, the
- older one wins. */
- (void) 0;
-
- return true;
- }
-
- /* Now we exclusively deal with VAR_DECLs. */
-
/* Sharing a global symbol is a strong hint that two types are
compatible. We could use this information to complete
incomplete pointed-to types more aggressively here, ignoring
@@ -254,19 +225,22 @@ lto_symtab_merge (symtab_node *prevailin
??? In principle we might want to only warn for structurally
incompatible types here, but unless we have protective measures
for TBAA in place that would hide useful information. */
- prevailing_type = TYPE_MAIN_VARIANT (TREE_TYPE (prevailing_decl));
- type = TYPE_MAIN_VARIANT (TREE_TYPE (decl));
+ prevailing_type = TYPE_MAIN_VARIANT (prevailing_type);
+ type = TYPE_MAIN_VARIANT (type);
if (!types_compatible_p (prevailing_type, type))
{
+ if (TREE_CODE (prevailing_type) == FUNCTION_TYPE
+ || TREE_CODE (type) == METHOD_TYPE)
+ return true;
if (COMPLETE_TYPE_P (type))
- return false;
+ return true;
/* If type is incomplete then avoid warnings in the cases
that TBAA handles just fine. */
if (TREE_CODE (prevailing_type) != TREE_CODE (type))
- return false;
+ return true;
if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
{
@@ -280,10 +254,10 @@ lto_symtab_merge (symtab_node *prevailin
}
if (TREE_CODE (tem1) != TREE_CODE (tem2))
- return false;
+ return true;
if (!types_compatible_p (tem1, tem2))
- return false;
+ return true;
}
/* Fallthru. Compatible enough. */
@@ -292,6 +266,43 @@ lto_symtab_merge (symtab_node *prevailin
/* ??? We might want to emit a warning here if type qualification
differences were spotted. Do not do this unconditionally though. */
+ return false;
+}
+
+/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
+ Return false if the symbols are not fully compatible and a diagnostic
+ should be emitted. */
+
+static bool
+lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
+{
+ tree prevailing_decl = prevailing->decl;
+ tree decl = entry->decl;
+
+ if (prevailing_decl == decl)
+ return true;
+
+ /* Merge decl state in both directions, we may still end up using
+ the new decl. */
+ TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
+ TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
+
+ /* The linker may ask us to combine two incompatible symbols.
+ Detect this case and notify the caller of required diagnostics. */
+
+ if (TREE_CODE (decl) == FUNCTION_DECL)
+ {
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
+ TREE_TYPE (decl)))
+ return false;
+
+ return true;
+ }
+
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
+ TREE_TYPE (decl)))
+ return false;
+
/* There is no point in comparing too many details of the decls here.
The type compatibility checks or the completing of types has properly
dealt with most issues. */
@@ -483,12 +494,18 @@ lto_symtab_merge_decls_2 (symtab_node *f
/* Diagnose all mismatched re-declarations. */
FOR_EACH_VEC_ELT (mismatches, i, decl)
{
- if (!types_compatible_p (TREE_TYPE (prevailing->decl),
- TREE_TYPE (decl)))
- diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
- "type of %qD does not match original "
- "declaration", decl);
-
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
+ TREE_TYPE (decl)))
+ {
+ bool diag;
+ diag = warning_at (DECL_SOURCE_LOCATION (decl), 0,
+ "type of %qD does not match original "
+ "declaration", decl);
+ diagnosed_p |= diag;
+ if (diag)
+ warn_types_mismatch (TREE_TYPE (prevailing->decl),
+ TREE_TYPE (decl));
+ }
else if ((DECL_USER_ALIGN (prevailing->decl)
&& DECL_USER_ALIGN (decl))
&& DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-04-27 8:34 Improve LTO type checking during symtab merging Jan Hubicka
2015-04-27 8:54 ` Richard Biener
2015-04-28 2:53 ` Jan Hubicka
@ 2015-04-28 4:07 ` Jan Hubicka
2015-04-28 7:15 ` Tobias Burnus
2015-04-28 8:14 ` Richard Biener
2 siblings, 2 replies; 10+ messages in thread
From: Jan Hubicka @ 2015-04-28 4:07 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, rguenther, burnus
Hi,
actually I bootstrapped/regtested without fortran (as I frogot the setting from
LTO bootstrap). There are several new warnings in the fortran's testsuite.
As far as I can tell, they represent real mismatches. I wonder, do we want
to fix the testcases (some fortran-fu would be needed), disable warnings on those
or explicitely allow excess warnings (bcause we still have no way to match
link-time warnings)?
Honza
spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_20091028-1_0.o f_lto_20091028-1_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -r -nostdlib -finline-functions -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-20091028-1-01.exe
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/20091028-1_0.f90:7:0: warning: type of 'int_gen_ti_header_c' does not match original declaration
lto1: note: return value type mismatch
lto1: note: type 'int' should match type 'void'
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/20091028-1_1.c:3:5: note: previously declared here
spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr40724_0.o f_lto_pr40724_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr40724-01.exe
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr40724_1.f:2:0: warning: type of 'f' does not match original declaration
lto1: note: types have different parameter counts
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr40724_0.f:1:0: note: previously declared here
spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr41069_0.o f_lto_pr41069_1.o f_lto_pr41069_2.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr41069-01.exe
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_1.f90:4:0: warning: type of 'mltfftsg' does not match original declaration
lto1: note: types have different parameter counts
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_1.f90:4:0: warning: type of 'mltfftsg' does not match original declaration
lto1: note: types have different parameter counts
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_0.f90:2:0: note: previously declared here
spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr41521_0.o f_lto_pr41521_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -g -O -flto -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr41521-11.exe
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41521_0.f90:7:0: warning: type of 'atom' does not match original declaration
lto1: note: types have different parameter counts
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41521_1.f90:1:0: note: previously declared here
spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr41576_0.o f_lto_pr41576_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -flto -Werror -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr41576-01.exe
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41576_1.f90:4:0: error: type of 'foo' does not match original declaration [-Werror]
lto1: note: types have different parameter counts
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41576_0.f90:4:0: note: previously declared here
lto1: all warnings being treated as errors
lto-wrapper: fatal error: /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../gfortran returned 1 exit status
compilation terminated.
/usr/bin/ld: fatal error: lto-wrapper failed
collect2: error: ld returned 1 exit status
compiler exited with status 1
spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr60635_0.o f_lto_pr60635_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr60635-01.exe
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr60635_0.f90:13:0: warning: type of 'bigendc16' does not match original declaration
lto1: note: return value type mismatch
/usr/include/stdint.h:50:28: note: type 'uint16_t' should match type 'short int'
/aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr60635_1.c:6:10: note: previously declared here
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-04-28 4:07 ` Jan Hubicka
@ 2015-04-28 7:15 ` Tobias Burnus
2015-04-28 8:14 ` Richard Biener
1 sibling, 0 replies; 10+ messages in thread
From: Tobias Burnus @ 2015-04-28 7:15 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, rguenther
Hi all,
Jan Hubicka wrote:
> actually I bootstrapped/regtested without fortran (as I frogot the setting from
> LTO bootstrap). There are several new warnings in the fortran's testsuite.
> As far as I can tell, they represent real mismatches. I wonder, do we want
> to fix the testcases (some fortran-fu would be needed), disable warnings on those
> or explicitely allow excess warnings (bcause we still have no way to match
> link-time warnings)?
See comments/solutions below. I wrote them bottom up. I think one can
fix all of them as stated, but I am not 100% sure whether some of the
cases explicitly should handle some mismatch - especially not regarding
the last one. But at least the rest should be fixable as stated.
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/20091028-1_0.f90:7:0: warning: type of 'int_gen_ti_header_c' does not match original declaration
> lto1: note: return value type mismatch
> lto1: note: type 'int' should match type 'void'
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/20091028-1_1.c:3:5: note: previously declared here
Here, one seemingly tries to ignore the return value of a function as it
is commonly done under C – except that Fortran strictly divides between
void-returning "subroutine"s and non-void-returning "function"s. Thus,
one has to declare the function in that case as "subroutine" and cause a
mismatch.
Assuming the test case doesn't test whether this is gracefully handled,
the simplest would be to change the C function to return void - in
particular as that this actually happens as there is no "return".
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr40724_1.f:2:0: warning: type of 'f' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr40724_0.f:1:0: note: previously declared here
See next items.
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_1.f90:4:0: warning: type of 'mltfftsg' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_1.f90:4:0: warning: type of 'mltfftsg' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_0.f90:2:0: note: previously declared here
I would add an interface (cf. next items); however, I wonder whether any
of those test cases relies on having no interface.
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41521_0.f90:7:0: warning: type of 'atom' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41521_1.f90:1:0: note: previously declared here
Reason: see next item. Solution: Add to pr41521_0.f90:
interface
subroutine atom(sol,k,eval)
real, intent(in) :: sol
integer, intent(in) :: k(2)
real, intent(out) :: eval(2)
end subroutine
end interface
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41576_1.f90:4:0: error: type of 'foo' does not match original declaration [-Werror]
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41576_0.f90:4:0: note: previously declared here
Here the problem is that gfortran doesn't know the interface of "foo" -
and declares it as "foo(...)". What the FE should do is to generate the
interface by the usage. [There is some PR for this.] For the test case,
one can simply add in pr41576_1.f90 the following:
interface
subroutine foo()
end subroutine
end interface
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr60635_0.f90:13:0: warning: type of 'bigendc16' does not match original declaration
> /usr/include/stdint.h:50:28: note: type 'uint16_t' should match type 'short int'
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr60635_1.c:6:10: note: previously declared here
The problem here is that the C side uses uint16_t; but Fortran doesn't
have unsigned integers. If only the return value is the problem, I think
one can simply cast it to "(int16_t)". But I don't know whether it then
still tests the original issue (it probably does).
Tobias
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-04-28 4:07 ` Jan Hubicka
2015-04-28 7:15 ` Tobias Burnus
@ 2015-04-28 8:14 ` Richard Biener
2015-05-11 4:45 ` Jan Hubicka
1 sibling, 1 reply; 10+ messages in thread
From: Richard Biener @ 2015-04-28 8:14 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, burnus
On Tue, 28 Apr 2015, Jan Hubicka wrote:
> Hi,
> actually I bootstrapped/regtested without fortran (as I frogot the setting from
> LTO bootstrap). There are several new warnings in the fortran's testsuite.
> As far as I can tell, they represent real mismatches. I wonder, do we want
> to fix the testcases (some fortran-fu would be needed), disable warnings on those
> or explicitely allow excess warnings (bcause we still have no way to match
> link-time warnings)?
Please disable the warnigns by default unless -Wodr is given. I also
explicitely pointed you to an existing PR with Fortran vs. C
interoperability... so you could have double-checked.
There is still the
FAIL: g++.dg/tree-ssa/pr61034.C -std=gnu++11 scan-tree-dump-times fre2
"free"
18
FAIL: g++.dg/tree-ssa/pr61034.C -std=gnu++14 scan-tree-dump-times fre2
"free"
18
FAIL: g++.dg/tree-ssa/pr61034.C -std=gnu++98 scan-tree-dump-times fre2
"free"
from one of your patches. Please make sure you fix testsuite fallout
you cause quickly.
Richard.
> Honza
>
> spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_20091028-1_0.o f_lto_20091028-1_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -r -nostdlib -finline-functions -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-20091028-1-01.exe
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/20091028-1_0.f90:7:0: warning: type of 'int_gen_ti_header_c' does not match original declaration
> lto1: note: return value type mismatch
> lto1: note: type 'int' should match type 'void'
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/20091028-1_1.c:3:5: note: previously declared here
>
> spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr40724_0.o f_lto_pr40724_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr40724-01.exe
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr40724_1.f:2:0: warning: type of 'f' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr40724_0.f:1:0: note: previously declared here
>
> spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr41069_0.o f_lto_pr41069_1.o f_lto_pr41069_2.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr41069-01.exe
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_1.f90:4:0: warning: type of 'mltfftsg' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_1.f90:4:0: warning: type of 'mltfftsg' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41069_0.f90:2:0: note: previously declared here
>
> spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran3/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr41521_0.o f_lto_pr41521_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -g -O -flto -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr41521-11.exe
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41521_0.f90:7:0: warning: type of 'atom' does not match original declaration
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41521_1.f90:1:0: note: previously declared here
>
> spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr41576_0.o f_lto_pr41576_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -flto -Werror -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr41576-01.exe
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41576_1.f90:4:0: error: type of 'foo' does not match original declaration [-Werror]
> lto1: note: types have different parameter counts
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr41576_0.f90:4:0: note: previously declared here
> lto1: all warnings being treated as errors
> lto-wrapper: fatal error: /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../gfortran returned 1 exit status
> compilation terminated.
> /usr/bin/ld: fatal error: lto-wrapper failed
> collect2: error: ld returned 1 exit status
> compiler exited with status 1
>
> spawn /aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../gfortran -B/aux/hubicka/trunk-8/build5/gcc/testsuite/gfortran11/../../ -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/ f_lto_pr60635_0.o f_lto_pr60635_1.o -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -flto -flto-partition=none -fuse-linker-plugin -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libgfortran/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libatomic/.libs -B/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -L/aux/hubicka/trunk-8/build5/x86_64-unknown-linux-gnu/./libquadmath/.libs -o gfortran-dg-lto-pr60635-01.exe
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr60635_0.f90:13:0: warning: type of 'bigendc16' does not match original declaration
> lto1: note: return value type mismatch
> /usr/include/stdint.h:50:28: note: type 'uint16_t' should match type 'short int'
> /aux/hubicka/trunk-8/gcc/testsuite/gfortran.dg/lto/pr60635_1.c:6:10: note: previously declared here
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-04-27 8:54 ` Richard Biener
@ 2015-04-28 14:42 ` Jan Hubicka
0 siblings, 0 replies; 10+ messages in thread
From: Jan Hubicka @ 2015-04-28 14:42 UTC (permalink / raw)
To: Richard Biener; +Cc: Jan Hubicka, gcc-patches
>
> Only emitting the warnings with -Wodr looks good to me. I can't see
> how we can decide what cases lead to wrong code surprises and what not
OK, I will go with -Wodr for all the warnings then, that seems fine to me.
> (other than using types_compatible_p ...). Wrong-code can only(?) happen
> if we happen to inline in a way that makes the incosistency visible in
> a single function.
>
> > Incrementally I am heading towards proper definition of decl
> > compatibility that I can plug into symtab merging and avoid merging
> > incompatible decls (so FORTIFY_SOURCE works).
> >
> > lto-symtab and ipa-icf both have some knowledge of the problem, I want to get
> > both right and factor out common logic.
>
> Sounds good.
>
> > Other improvement is to preserve the ODR type info when non-ODR variant
> > previals so one can diagnose clash in between C++ units even in mixed
> > language linktimes.
>
> Hmm, but then merging ODR with non-ODR variant is already pointing to
> a ODR violation? So why do you need to retain that info?
non-ODR type is compatible with all ODR types that are structurally equivalent,
but these ODR types may not be compatible with each other. For example:
a.c
struct a {int a;} var;
b.C
extern struct a {int a;} var;
c.C
extern struct b {int a;} var;
has ODR violatio nbetween b.C and c.C, but because the variable is defined
in C source file, we will only check compatibility with non-ODR struct a.
My plan is to simply record if the declaration also has ODR type associated
with it. But that is for incremental improvement.
>
> Btw, there is that old PR41227 which has both wrong-code and diagnostic
> impact...
Thanks, I will take deeper look. I did not know that fortran has way
to mark types that are supposed to interpoerate outside fortran units.
This may be potentially interesting.
Honza
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-04-28 8:14 ` Richard Biener
@ 2015-05-11 4:45 ` Jan Hubicka
2015-05-12 10:04 ` Richard Biener
2015-05-17 10:42 ` Andreas Schwab
0 siblings, 2 replies; 10+ messages in thread
From: Jan Hubicka @ 2015-05-11 4:45 UTC (permalink / raw)
To: Richard Biener; +Cc: Jan Hubicka, gcc-patches, burnus
Hi,
this is the updated version of patch with testsuite compensation and
some changes:
- As discussed with Richard on IRC, I added -Wlto-type-mismatch to control
the warnings (enabled by default) and split ODR warnings from warnings
about incompatible types (in a sense of weak structural equality tested by
types_compatible_p at LTO time).
Both -Wlto-type-mismatch and -Wodr are on by default. -Wodr points will
output warnings only on conflicts between C++ programs that are positively
undefined by the standard (modulo implementation bugs) and
-Wlto-type-mismatch positives points out likely wrong code since the
declarations are not compatible to -fstrit-aliasing.
The testuiste fallout of Fortran was not that hard to fix, so I hope
-Wlto-type-mismatch is weak enough to make sense for mixed language
units. In fact I can use the ODR type matching tocde to implement
more strict checks for separate flag (-Wlto-strict-type-mismatch)
that would be off by default or perhaps just warn between C and C++
units.
- I got Firefox building and noticed a false positives on functions
when forward declaration and prototype was mixed. THis is because
useless_type_conversion compares function types but requires the outer
type to be the one more specified (prototype).
types_compatible_p checks that the types are convertible both directions
so it return false.
This whole code does not make much sense to be. I do not see why
useless_type_conversion needs to care about funcition types - we never
convert these. I also do not see why it match TYPE_METHOD_BASETYPE
when this field is never used for codegen. It is used by ubsan
and dbxout only.
I think useless_type_conversion can jsut ICE on function/method types
and types_compatible_p can be extended by same code as I have
in warn_types_mismatch minus the TYPE_METHOD_BASETYPE matching.
On a positive note, the patch also finds some real bugs in Firefox :)
- I also noticed that we may miss some ODR warnings when the declaration
is of compound type, not named type. (i.e. odr_type a[4];).
To make these working I added odr_or_derived_type_p and exported
the functionality to make structural compare from ipa-devirt.
The way of walking compount types was discussed with Jason here:
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00382.html
Bootstrapped/regtested x86_64-linux, ppc64 running. Also tested with ltobootstrap.
OK?
* ipa-utils.h (warn_types_mismatch, odr_or_derived_type_p,
odr_types_equivalent_p): Declare.
(odr_type_p): Use gcc_checking_assert.
* common.opt (Wlto-type-mismatch): New warning.
* ipa-devirt.c (compound_type_base): New function.
(odr_or_derived_type_p): New function.
(odr_types_equivalent_p): New function.
(add_type_duplicate): Simplify.
* lto-symtab.c (warn_type_compatibility_p): Break out from ...;
compare ODR types (if available) and function types.
(lto_symtab_merge): ... here; output ODR violation warnings
and call warn_types_mismatch.
* gfortran.dg/lto/20091028-2_1.c: Fix return value.
* gfortran.dg/lto/pr41576_1.f90: Add interface.
* gfortran.dg/lto/pr41521_0.f90: Disable lto-type-mismatch
* gfortran.dg/lto/pr60635_0.f90: Disable lto-type-mismatch.
* gfortran.dg/lto/20091028-1_1.c: Fix return type.
* gcc.dg/lto/20120723_0.c: Disbale lto-type-mismatch.
Index: ipa-utils.h
===================================================================
--- ipa-utils.h (revision 222991)
+++ ipa-utils.h (working copy)
@@ -84,6 +84,9 @@ bool types_must_be_same_for_odr (tree, t
bool types_odr_comparable (tree, tree, bool strict = false);
cgraph_node *try_speculative_devirtualization (tree, HOST_WIDE_INT,
ipa_polymorphic_call_context);
+void warn_types_mismatch (tree t1, tree t2);
+bool odr_or_derived_type_p (const_tree t);
+bool odr_types_equivalent_p (tree type1, tree type2);
/* Return vector containing possible targets of polymorphic call E.
If COMPLETEP is non-NULL, store true if the list is complete.
@@ -164,7 +167,7 @@ odr_type_p (const_tree t)
return true;
/* We do not have this information when not in LTO, but we do not need
to care, since it is used only for type merging. */
- gcc_assert (in_lto_p || flag_lto);
+ gcc_checking_assert (in_lto_p || flag_lto);
return (TYPE_NAME (t)
&& (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))));
Index: common.opt
===================================================================
--- common.opt (revision 222991)
+++ common.opt (working copy)
@@ -607,6 +607,10 @@ Woverflow
Common Var(warn_overflow) Init(1) Warning
Warn about overflow in arithmetic expressions
+Wlto-type-mismatch
+Common Var(warn_lto_type_mismatch) Init(1) Warning
+During link time optimization warn about mismatched types of global declarations
+
Wpacked
Common Var(warn_packed) Warning
Warn when the packed attribute has no effect on struct layout
Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c (revision 222991)
+++ lto/lto-symtab.c (working copy)
@@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.
#include "ipa-prop.h"
#include "ipa-inline.h"
#include "builtins.h"
+#include "print-tree.h"
/* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
all edges and removing the old node. */
@@ -203,45 +204,49 @@ lto_varpool_replace_node (varpool_node *
vnode->remove ();
}
-/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
- Return false if the symbols are not fully compatible and a diagnostic
- should be emitted. */
+/* Return non-zero if we want to output waring about T1 and T2.
+ Return value is a bitmask of reasons of violation:
+ Bit 0 indicates that types are not compatible of memory layout.
+ Bot 1 indicates that types are not compatible because of C++ ODR rule. */
-static bool
-lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
+static int
+warn_type_compatibility_p (tree prevailing_type, tree type)
{
- tree prevailing_decl = prevailing->decl;
- tree decl = entry->decl;
- tree prevailing_type, type;
-
- if (prevailing_decl == decl)
- return true;
-
- /* Merge decl state in both directions, we may still end up using
- the new decl. */
- TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
- TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
-
- /* The linker may ask us to combine two incompatible symbols.
- Detect this case and notify the caller of required diagnostics. */
-
- if (TREE_CODE (decl) == FUNCTION_DECL)
- {
- if (!types_compatible_p (TREE_TYPE (prevailing_decl),
- TREE_TYPE (decl)))
- /* If we don't have a merged type yet...sigh. The linker
- wouldn't complain if the types were mismatched, so we
- probably shouldn't either. Just use the type from
- whichever decl appears to be associated with the
- definition. If for some odd reason neither decl is, the
- older one wins. */
- (void) 0;
-
- return true;
+ int lev = 0;
+ /* C++ provide a robust way to check for type compatibility via the ODR
+ rule. */
+ if (odr_or_derived_type_p (prevailing_type) && odr_type_p (type)
+ && !odr_types_equivalent_p (prevailing_type, type))
+ lev = 2;
+
+ /* Function types needs special care, because types_compatible_p never
+ thinks prototype is compatible to non-prototype. */
+ if ((TREE_CODE (type) == FUNCTION_TYPE || TREE_CODE (type) == METHOD_TYPE)
+ && TREE_CODE (type) == TREE_CODE (prevailing_type))
+ {
+ lev |= warn_type_compatibility_p (TREE_TYPE (prevailing_type),
+ TREE_TYPE (type));
+ if (TREE_CODE (type) == METHOD_TYPE)
+ lev |= warn_type_compatibility_p (TYPE_METHOD_BASETYPE (prevailing_type),
+ TYPE_METHOD_BASETYPE (type));
+ if (prototype_p (prevailing_type) && prototype_p (type)
+ && TYPE_ARG_TYPES (prevailing_type) != TYPE_ARG_TYPES (type))
+ {
+ tree parm1, parm2;
+ for (parm1 = TYPE_ARG_TYPES (prevailing_type),
+ parm2 = TYPE_ARG_TYPES (type);
+ parm1 && parm2;
+ parm1 = TREE_CHAIN (prevailing_type),
+ parm2 = TREE_CHAIN (type))
+ lev |= warn_type_compatibility_p (TREE_VALUE (parm1),
+ TREE_VALUE (parm2));
+ if (parm1 || parm2)
+ lev = 3;
+ }
+ if (comp_type_attributes (prevailing_type, type) == 0)
+ lev = 3;
+ return lev;
}
-
- /* Now we exclusively deal with VAR_DECLs. */
-
/* Sharing a global symbol is a strong hint that two types are
compatible. We could use this information to complete
incomplete pointed-to types more aggressively here, ignoring
@@ -254,19 +259,22 @@ lto_symtab_merge (symtab_node *prevailin
??? In principle we might want to only warn for structurally
incompatible types here, but unless we have protective measures
for TBAA in place that would hide useful information. */
- prevailing_type = TYPE_MAIN_VARIANT (TREE_TYPE (prevailing_decl));
- type = TYPE_MAIN_VARIANT (TREE_TYPE (decl));
+ prevailing_type = TYPE_MAIN_VARIANT (prevailing_type);
+ type = TYPE_MAIN_VARIANT (type);
if (!types_compatible_p (prevailing_type, type))
{
- if (COMPLETE_TYPE_P (type))
- return false;
+ if (TREE_CODE (prevailing_type) == FUNCTION_TYPE
+ || TREE_CODE (type) == METHOD_TYPE)
+ return 1 | lev;
+ if (COMPLETE_TYPE_P (type) && COMPLETE_TYPE_P (prevailing_type))
+ return 1 | lev;
/* If type is incomplete then avoid warnings in the cases
that TBAA handles just fine. */
if (TREE_CODE (prevailing_type) != TREE_CODE (type))
- return false;
+ return 1 | lev;
if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
{
@@ -280,10 +288,10 @@ lto_symtab_merge (symtab_node *prevailin
}
if (TREE_CODE (tem1) != TREE_CODE (tem2))
- return false;
+ return 1 | lev;
if (!types_compatible_p (tem1, tem2))
- return false;
+ return 1 | lev;
}
/* Fallthru. Compatible enough. */
@@ -292,6 +300,43 @@ lto_symtab_merge (symtab_node *prevailin
/* ??? We might want to emit a warning here if type qualification
differences were spotted. Do not do this unconditionally though. */
+ return lev;
+}
+
+/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
+ Return false if the symbols are not fully compatible and a diagnostic
+ should be emitted. */
+
+static bool
+lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
+{
+ tree prevailing_decl = prevailing->decl;
+ tree decl = entry->decl;
+
+ if (prevailing_decl == decl)
+ return true;
+
+ /* Merge decl state in both directions, we may still end up using
+ the new decl. */
+ TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
+ TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
+
+ /* The linker may ask us to combine two incompatible symbols.
+ Detect this case and notify the caller of required diagnostics. */
+
+ if (TREE_CODE (decl) == FUNCTION_DECL)
+ {
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
+ TREE_TYPE (decl)))
+ return false;
+
+ return true;
+ }
+
+ if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
+ TREE_TYPE (decl)))
+ return false;
+
/* There is no point in comparing too many details of the decls here.
The type compatibility checks or the completing of types has properly
dealt with most issues. */
@@ -483,24 +528,39 @@ lto_symtab_merge_decls_2 (symtab_node *f
/* Diagnose all mismatched re-declarations. */
FOR_EACH_VEC_ELT (mismatches, i, decl)
{
- if (!types_compatible_p (TREE_TYPE (prevailing->decl),
- TREE_TYPE (decl)))
- diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
- "type of %qD does not match original "
- "declaration", decl);
-
+ int level = warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
+ TREE_TYPE (decl));
+ if (level)
+ {
+ bool diag = false;
+ if (level > 1)
+ diag = warning_at (DECL_SOURCE_LOCATION (decl),
+ OPT_Wodr,
+ "%qD violates the C++ One Definition Rule ",
+ decl);
+ if (!diag && (level & 1))
+ diag = warning_at (DECL_SOURCE_LOCATION (decl),
+ OPT_Wlto_type_mismatch,
+ "type of %qD does not match original "
+ "declaration", decl);
+ if (diag)
+ warn_types_mismatch (TREE_TYPE (prevailing->decl),
+ TREE_TYPE (decl));
+ diagnosed_p |= diag;
+ }
else if ((DECL_USER_ALIGN (prevailing->decl)
&& DECL_USER_ALIGN (decl))
&& DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
{
- diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
+ diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl),
+ OPT_Wlto_type_mismatch,
"alignment of %qD is bigger than "
"original declaration", decl);
}
}
if (diagnosed_p)
inform (DECL_SOURCE_LOCATION (prevailing->decl),
- "previously declared here");
+ "%qD was previously declared here", prevailing->decl);
mismatches.release ();
}
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c (revision 222991)
+++ ipa-devirt.c (working copy)
@@ -549,6 +549,59 @@ types_must_be_same_for_odr (tree t1, tre
return TYPE_MAIN_VARIANT (t1) == TYPE_MAIN_VARIANT (t2);
}
+/* If T is compound type, return type it is based on. */
+
+static tree
+compound_type_base (const_tree t)
+{
+ if (TREE_CODE (t) == ARRAY_TYPE
+ || POINTER_TYPE_P (t)
+ || TREE_CODE (t) == COMPLEX_TYPE
+ || VECTOR_TYPE_P (t))
+ return TREE_TYPE (t);
+ if (TREE_CODE (t) == METHOD_TYPE)
+ return TYPE_METHOD_BASETYPE (t);
+ if (TREE_CODE (t) == OFFSET_TYPE)
+ return TYPE_OFFSET_BASETYPE (t);
+ return NULL_TREE;
+}
+
+/* Return true if T is either ODR type or compound type based from it.
+ If the function return true, we know that T is a type originating from C++
+ source even at link-time. */
+
+bool
+odr_or_derived_type_p (const_tree t)
+{
+ do
+ {
+ if (odr_type_p (t))
+ return true;
+ /* Function type is a tricky one. Basically we can consider it
+ ODR derived if return type or any of the parameters is.
+ We need to check all parameters because LTO streaming merges
+ common types (such as void) and they are not considered ODR then. */
+ if (TREE_CODE (t) == FUNCTION_TYPE)
+ {
+ if (TYPE_METHOD_BASETYPE (t))
+ t = TYPE_METHOD_BASETYPE (t);
+ else
+ {
+ if (TREE_TYPE (t) && odr_or_derived_type_p (TREE_TYPE (t)))
+ return true;
+ for (t = TYPE_ARG_TYPES (t); t; t = TREE_CHAIN (t))
+ if (odr_or_derived_type_p (TREE_VALUE (t)))
+ return true;
+ return false;
+ }
+ }
+ else
+ t = compound_type_base (t);
+ }
+ while (t);
+ return t;
+}
+
/* Compare types T1 and T2 and return true if they are
equivalent. */
@@ -1577,6 +1642,20 @@ odr_types_equivalent_p (tree t1, tree t2
return true;
}
+/* Return true if TYPE1 and TYPE2 are equivalent for One Definition Rule. */
+
+bool
+odr_types_equivalent_p (tree type1, tree type2)
+{
+ hash_set<type_pair,pair_traits> visited;
+
+#ifdef ENABLE_CHECKING
+ gcc_assert (odr_or_derived_type_p (type1) && odr_or_derived_type_p (type2));
+#endif
+ return odr_types_equivalent_p (type1, type2, false, NULL,
+ &visited);
+}
+
/* TYPE is equivalent to VAL by ODR, but its tree representation differs
from VAL->type. This may happen in LTO where tree merging did not merge
all variants of the same type or due to ODR violation.
@@ -1701,12 +1780,8 @@ add_type_duplicate (odr_type val, tree t
base_mismatch = true;
}
else
- {
- hash_set<type_pair,pair_traits> visited;
- if (!odr_types_equivalent_p (type1, type2, false, NULL,
- &visited))
- base_mismatch = true;
- }
+ if (!odr_types_equivalent_p (type1, type2))
+ base_mismatch = true;
if (base_mismatch)
{
if (!warned && !val->odr_violated)
Index: testsuite/gfortran.dg/lto/20091028-2_1.c
===================================================================
--- testsuite/gfortran.dg/lto/20091028-2_1.c (revision 222991)
+++ testsuite/gfortran.dg/lto/20091028-2_1.c (working copy)
@@ -1,9 +1,9 @@
extern void *memcpy(void *dest, const void *src, __SIZE_TYPE__ n);
char *p;
-int int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
- int * itypesize, int * typesize,
- int * DataHandle, char * Data,
- int * Count, int * code)
+void int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
+ int * itypesize, int * typesize,
+ int * DataHandle, char * Data,
+ int * Count, int * code)
{
memcpy (typesize, p, sizeof(int)) ;
memcpy (Data, p, *Count * *typesize) ;
Index: testsuite/gfortran.dg/lto/pr41576_1.f90
===================================================================
--- testsuite/gfortran.dg/lto/pr41576_1.f90 (revision 222991)
+++ testsuite/gfortran.dg/lto/pr41576_1.f90 (working copy)
@@ -5,3 +5,8 @@ program test
if (c/=1 .or. d/=2) call abort
end program test
+interface
+ subroutine foo()
+ end subroutine
+end interface
+
Index: testsuite/gfortran.dg/lto/pr41521_0.f90
===================================================================
--- testsuite/gfortran.dg/lto/pr41521_0.f90 (revision 222991)
+++ testsuite/gfortran.dg/lto/pr41521_0.f90 (working copy)
@@ -1,9 +1,9 @@
! { dg-lto-do link }
-! { dg-lto-options {{-g -flto} {-g -O -flto}} }
+! { dg-lto-options {{-g -flto -Wno-lto-type-mismatch} {-g -O -flto -Wno-lto-type-mismatch}} }
program species
integer spk(2)
real eval(2)
spk = 2
call atom(1.1,spk,eval)
end program
Index: testsuite/gfortran.dg/lto/pr60635_0.f90
===================================================================
--- testsuite/gfortran.dg/lto/pr60635_0.f90 (revision 222991)
+++ testsuite/gfortran.dg/lto/pr60635_0.f90 (working copy)
@@ -1,4 +1,5 @@
! { dg-lto-do link }
+! { dg-lto-options {{ -Wno-lto-type-mismatch }} }
program test
use iso_fortran_env
Index: testsuite/gfortran.dg/lto/pr41576_0.f90
===================================================================
--- testsuite/gfortran.dg/lto/pr41576_0.f90 (revision 222991)
+++ testsuite/gfortran.dg/lto/pr41576_0.f90 (working copy)
@@ -1,5 +1,5 @@
! { dg-lto-do run }
-! { dg-lto-options { { -O2 -flto -Werror } } }
+! { dg-lto-options { { -O2 -flto -Werror -Wno-lto-type-mismatch } } }
subroutine foo
common /bar/ a, b
Index: testsuite/gfortran.dg/lto/20091028-1_1.c
===================================================================
--- testsuite/gfortran.dg/lto/20091028-1_1.c (revision 222991)
+++ testsuite/gfortran.dg/lto/20091028-1_1.c (working copy)
@@ -1,9 +1,9 @@
extern void bcopy(const void *, void *, __SIZE_TYPE__ n);
char *p;
-int int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
- int * itypesize, int * typesize,
- int * DataHandle, char * Data,
- int * Count, int * code)
+void int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
+ int * itypesize, int * typesize,
+ int * DataHandle, char * Data,
+ int * Count, int * code)
{
bcopy (typesize, p, sizeof(int)) ;
bcopy (Data, p, *Count * *typesize) ;
Index: testsuite/gcc.dg/lto/20120723_0.c
===================================================================
--- testsuite/gcc.dg/lto/20120723_0.c (revision 222991)
+++ testsuite/gcc.dg/lto/20120723_0.c (working copy)
@@ -3,7 +3,7 @@
??? This testcase is invalid C and can only pass on specific platforms. */
/* { dg-lto-do run } */
/* { dg-skip-if "" { { sparc*-*-* } && ilp32 } { "*" } { "" } } */
-/* { dg-lto-options { {-O3 -fno-early-inlining -flto}} } */
+/* { dg-lto-options { {-O3 -fno-early-inlining -flto -Wno-lto-type-mismatch}} } */
extern void abort (void);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-05-11 4:45 ` Jan Hubicka
@ 2015-05-12 10:04 ` Richard Biener
2015-05-17 10:42 ` Andreas Schwab
1 sibling, 0 replies; 10+ messages in thread
From: Richard Biener @ 2015-05-12 10:04 UTC (permalink / raw)
To: Jan Hubicka; +Cc: Richard Biener, GCC Patches, Tobias Burnus
On Mon, May 11, 2015 at 6:42 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this is the updated version of patch with testsuite compensation and
> some changes:
> - As discussed with Richard on IRC, I added -Wlto-type-mismatch to control
> the warnings (enabled by default) and split ODR warnings from warnings
> about incompatible types (in a sense of weak structural equality tested by
> types_compatible_p at LTO time).
>
> Both -Wlto-type-mismatch and -Wodr are on by default. -Wodr points will
> output warnings only on conflicts between C++ programs that are positively
> undefined by the standard (modulo implementation bugs) and
> -Wlto-type-mismatch positives points out likely wrong code since the
> declarations are not compatible to -fstrit-aliasing.
>
> The testuiste fallout of Fortran was not that hard to fix, so I hope
> -Wlto-type-mismatch is weak enough to make sense for mixed language
> units. In fact I can use the ODR type matching tocde to implement
> more strict checks for separate flag (-Wlto-strict-type-mismatch)
> that would be off by default or perhaps just warn between C and C++
> units.
> - I got Firefox building and noticed a false positives on functions
> when forward declaration and prototype was mixed. THis is because
> useless_type_conversion compares function types but requires the outer
> type to be the one more specified (prototype).
> types_compatible_p checks that the types are convertible both directions
> so it return false.
>
> This whole code does not make much sense to be. I do not see why
> useless_type_conversion needs to care about funcition types - we never
> convert these. I also do not see why it match TYPE_METHOD_BASETYPE
> when this field is never used for codegen. It is used by ubsan
> and dbxout only.
>
> I think useless_type_conversion can jsut ICE on function/method types
> and types_compatible_p can be extended by same code as I have
> in warn_types_mismatch minus the TYPE_METHOD_BASETYPE matching.
>
> On a positive note, the patch also finds some real bugs in Firefox :)
> - I also noticed that we may miss some ODR warnings when the declaration
> is of compound type, not named type. (i.e. odr_type a[4];).
> To make these working I added odr_or_derived_type_p and exported
> the functionality to make structural compare from ipa-devirt.
> The way of walking compount types was discussed with Jason here:
> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00382.html
>
> Bootstrapped/regtested x86_64-linux, ppc64 running. Also tested with ltobootstrap.
> OK?
Looks good to me.
Richard.
> * ipa-utils.h (warn_types_mismatch, odr_or_derived_type_p,
> odr_types_equivalent_p): Declare.
> (odr_type_p): Use gcc_checking_assert.
> * common.opt (Wlto-type-mismatch): New warning.
> * ipa-devirt.c (compound_type_base): New function.
> (odr_or_derived_type_p): New function.
> (odr_types_equivalent_p): New function.
> (add_type_duplicate): Simplify.
>
> * lto-symtab.c (warn_type_compatibility_p): Break out from ...;
> compare ODR types (if available) and function types.
> (lto_symtab_merge): ... here; output ODR violation warnings
> and call warn_types_mismatch.
>
> * gfortran.dg/lto/20091028-2_1.c: Fix return value.
> * gfortran.dg/lto/pr41576_1.f90: Add interface.
> * gfortran.dg/lto/pr41521_0.f90: Disable lto-type-mismatch
> * gfortran.dg/lto/pr60635_0.f90: Disable lto-type-mismatch.
> * gfortran.dg/lto/20091028-1_1.c: Fix return type.
> * gcc.dg/lto/20120723_0.c: Disbale lto-type-mismatch.
> Index: ipa-utils.h
> ===================================================================
> --- ipa-utils.h (revision 222991)
> +++ ipa-utils.h (working copy)
> @@ -84,6 +84,9 @@ bool types_must_be_same_for_odr (tree, t
> bool types_odr_comparable (tree, tree, bool strict = false);
> cgraph_node *try_speculative_devirtualization (tree, HOST_WIDE_INT,
> ipa_polymorphic_call_context);
> +void warn_types_mismatch (tree t1, tree t2);
> +bool odr_or_derived_type_p (const_tree t);
> +bool odr_types_equivalent_p (tree type1, tree type2);
>
> /* Return vector containing possible targets of polymorphic call E.
> If COMPLETEP is non-NULL, store true if the list is complete.
> @@ -164,7 +167,7 @@ odr_type_p (const_tree t)
> return true;
> /* We do not have this information when not in LTO, but we do not need
> to care, since it is used only for type merging. */
> - gcc_assert (in_lto_p || flag_lto);
> + gcc_checking_assert (in_lto_p || flag_lto);
>
> return (TYPE_NAME (t)
> && (DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))));
> Index: common.opt
> ===================================================================
> --- common.opt (revision 222991)
> +++ common.opt (working copy)
> @@ -607,6 +607,10 @@ Woverflow
> Common Var(warn_overflow) Init(1) Warning
> Warn about overflow in arithmetic expressions
>
> +Wlto-type-mismatch
> +Common Var(warn_lto_type_mismatch) Init(1) Warning
> +During link time optimization warn about mismatched types of global declarations
> +
> Wpacked
> Common Var(warn_packed) Warning
> Warn when the packed attribute has no effect on struct layout
> Index: lto/lto-symtab.c
> ===================================================================
> --- lto/lto-symtab.c (revision 222991)
> +++ lto/lto-symtab.c (working copy)
> @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.
> #include "ipa-prop.h"
> #include "ipa-inline.h"
> #include "builtins.h"
> +#include "print-tree.h"
>
> /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
> all edges and removing the old node. */
> @@ -203,45 +204,49 @@ lto_varpool_replace_node (varpool_node *
> vnode->remove ();
> }
>
> -/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
> - Return false if the symbols are not fully compatible and a diagnostic
> - should be emitted. */
> +/* Return non-zero if we want to output waring about T1 and T2.
> + Return value is a bitmask of reasons of violation:
> + Bit 0 indicates that types are not compatible of memory layout.
> + Bot 1 indicates that types are not compatible because of C++ ODR rule. */
>
> -static bool
> -lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +static int
> +warn_type_compatibility_p (tree prevailing_type, tree type)
> {
> - tree prevailing_decl = prevailing->decl;
> - tree decl = entry->decl;
> - tree prevailing_type, type;
> -
> - if (prevailing_decl == decl)
> - return true;
> -
> - /* Merge decl state in both directions, we may still end up using
> - the new decl. */
> - TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
> - TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
> -
> - /* The linker may ask us to combine two incompatible symbols.
> - Detect this case and notify the caller of required diagnostics. */
> -
> - if (TREE_CODE (decl) == FUNCTION_DECL)
> - {
> - if (!types_compatible_p (TREE_TYPE (prevailing_decl),
> - TREE_TYPE (decl)))
> - /* If we don't have a merged type yet...sigh. The linker
> - wouldn't complain if the types were mismatched, so we
> - probably shouldn't either. Just use the type from
> - whichever decl appears to be associated with the
> - definition. If for some odd reason neither decl is, the
> - older one wins. */
> - (void) 0;
> -
> - return true;
> + int lev = 0;
> + /* C++ provide a robust way to check for type compatibility via the ODR
> + rule. */
> + if (odr_or_derived_type_p (prevailing_type) && odr_type_p (type)
> + && !odr_types_equivalent_p (prevailing_type, type))
> + lev = 2;
> +
> + /* Function types needs special care, because types_compatible_p never
> + thinks prototype is compatible to non-prototype. */
> + if ((TREE_CODE (type) == FUNCTION_TYPE || TREE_CODE (type) == METHOD_TYPE)
> + && TREE_CODE (type) == TREE_CODE (prevailing_type))
> + {
> + lev |= warn_type_compatibility_p (TREE_TYPE (prevailing_type),
> + TREE_TYPE (type));
> + if (TREE_CODE (type) == METHOD_TYPE)
> + lev |= warn_type_compatibility_p (TYPE_METHOD_BASETYPE (prevailing_type),
> + TYPE_METHOD_BASETYPE (type));
> + if (prototype_p (prevailing_type) && prototype_p (type)
> + && TYPE_ARG_TYPES (prevailing_type) != TYPE_ARG_TYPES (type))
> + {
> + tree parm1, parm2;
> + for (parm1 = TYPE_ARG_TYPES (prevailing_type),
> + parm2 = TYPE_ARG_TYPES (type);
> + parm1 && parm2;
> + parm1 = TREE_CHAIN (prevailing_type),
> + parm2 = TREE_CHAIN (type))
> + lev |= warn_type_compatibility_p (TREE_VALUE (parm1),
> + TREE_VALUE (parm2));
> + if (parm1 || parm2)
> + lev = 3;
> + }
> + if (comp_type_attributes (prevailing_type, type) == 0)
> + lev = 3;
> + return lev;
> }
> -
> - /* Now we exclusively deal with VAR_DECLs. */
> -
> /* Sharing a global symbol is a strong hint that two types are
> compatible. We could use this information to complete
> incomplete pointed-to types more aggressively here, ignoring
> @@ -254,19 +259,22 @@ lto_symtab_merge (symtab_node *prevailin
> ??? In principle we might want to only warn for structurally
> incompatible types here, but unless we have protective measures
> for TBAA in place that would hide useful information. */
> - prevailing_type = TYPE_MAIN_VARIANT (TREE_TYPE (prevailing_decl));
> - type = TYPE_MAIN_VARIANT (TREE_TYPE (decl));
> + prevailing_type = TYPE_MAIN_VARIANT (prevailing_type);
> + type = TYPE_MAIN_VARIANT (type);
>
> if (!types_compatible_p (prevailing_type, type))
> {
> - if (COMPLETE_TYPE_P (type))
> - return false;
> + if (TREE_CODE (prevailing_type) == FUNCTION_TYPE
> + || TREE_CODE (type) == METHOD_TYPE)
> + return 1 | lev;
> + if (COMPLETE_TYPE_P (type) && COMPLETE_TYPE_P (prevailing_type))
> + return 1 | lev;
>
> /* If type is incomplete then avoid warnings in the cases
> that TBAA handles just fine. */
>
> if (TREE_CODE (prevailing_type) != TREE_CODE (type))
> - return false;
> + return 1 | lev;
>
> if (TREE_CODE (prevailing_type) == ARRAY_TYPE)
> {
> @@ -280,10 +288,10 @@ lto_symtab_merge (symtab_node *prevailin
> }
>
> if (TREE_CODE (tem1) != TREE_CODE (tem2))
> - return false;
> + return 1 | lev;
>
> if (!types_compatible_p (tem1, tem2))
> - return false;
> + return 1 | lev;
> }
>
> /* Fallthru. Compatible enough. */
> @@ -292,6 +300,43 @@ lto_symtab_merge (symtab_node *prevailin
> /* ??? We might want to emit a warning here if type qualification
> differences were spotted. Do not do this unconditionally though. */
>
> + return lev;
> +}
> +
> +/* Merge two variable or function symbol table entries PREVAILING and ENTRY.
> + Return false if the symbols are not fully compatible and a diagnostic
> + should be emitted. */
> +
> +static bool
> +lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)
> +{
> + tree prevailing_decl = prevailing->decl;
> + tree decl = entry->decl;
> +
> + if (prevailing_decl == decl)
> + return true;
> +
> + /* Merge decl state in both directions, we may still end up using
> + the new decl. */
> + TREE_ADDRESSABLE (prevailing_decl) |= TREE_ADDRESSABLE (decl);
> + TREE_ADDRESSABLE (decl) |= TREE_ADDRESSABLE (prevailing_decl);
> +
> + /* The linker may ask us to combine two incompatible symbols.
> + Detect this case and notify the caller of required diagnostics. */
> +
> + if (TREE_CODE (decl) == FUNCTION_DECL)
> + {
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> + TREE_TYPE (decl)))
> + return false;
> +
> + return true;
> + }
> +
> + if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> + TREE_TYPE (decl)))
> + return false;
> +
> /* There is no point in comparing too many details of the decls here.
> The type compatibility checks or the completing of types has properly
> dealt with most issues. */
> @@ -483,24 +528,39 @@ lto_symtab_merge_decls_2 (symtab_node *f
> /* Diagnose all mismatched re-declarations. */
> FOR_EACH_VEC_ELT (mismatches, i, decl)
> {
> - if (!types_compatible_p (TREE_TYPE (prevailing->decl),
> - TREE_TYPE (decl)))
> - diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
> - "type of %qD does not match original "
> - "declaration", decl);
> -
> + int level = warn_type_compatibility_p (TREE_TYPE (prevailing->decl),
> + TREE_TYPE (decl));
> + if (level)
> + {
> + bool diag = false;
> + if (level > 1)
> + diag = warning_at (DECL_SOURCE_LOCATION (decl),
> + OPT_Wodr,
> + "%qD violates the C++ One Definition Rule ",
> + decl);
> + if (!diag && (level & 1))
> + diag = warning_at (DECL_SOURCE_LOCATION (decl),
> + OPT_Wlto_type_mismatch,
> + "type of %qD does not match original "
> + "declaration", decl);
> + if (diag)
> + warn_types_mismatch (TREE_TYPE (prevailing->decl),
> + TREE_TYPE (decl));
> + diagnosed_p |= diag;
> + }
> else if ((DECL_USER_ALIGN (prevailing->decl)
> && DECL_USER_ALIGN (decl))
> && DECL_ALIGN (prevailing->decl) < DECL_ALIGN (decl))
> {
> - diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl), 0,
> + diagnosed_p |= warning_at (DECL_SOURCE_LOCATION (decl),
> + OPT_Wlto_type_mismatch,
> "alignment of %qD is bigger than "
> "original declaration", decl);
> }
> }
> if (diagnosed_p)
> inform (DECL_SOURCE_LOCATION (prevailing->decl),
> - "previously declared here");
> + "%qD was previously declared here", prevailing->decl);
>
> mismatches.release ();
> }
> Index: ipa-devirt.c
> ===================================================================
> --- ipa-devirt.c (revision 222991)
> +++ ipa-devirt.c (working copy)
> @@ -549,6 +549,59 @@ types_must_be_same_for_odr (tree t1, tre
> return TYPE_MAIN_VARIANT (t1) == TYPE_MAIN_VARIANT (t2);
> }
>
> +/* If T is compound type, return type it is based on. */
> +
> +static tree
> +compound_type_base (const_tree t)
> +{
> + if (TREE_CODE (t) == ARRAY_TYPE
> + || POINTER_TYPE_P (t)
> + || TREE_CODE (t) == COMPLEX_TYPE
> + || VECTOR_TYPE_P (t))
> + return TREE_TYPE (t);
> + if (TREE_CODE (t) == METHOD_TYPE)
> + return TYPE_METHOD_BASETYPE (t);
> + if (TREE_CODE (t) == OFFSET_TYPE)
> + return TYPE_OFFSET_BASETYPE (t);
> + return NULL_TREE;
> +}
> +
> +/* Return true if T is either ODR type or compound type based from it.
> + If the function return true, we know that T is a type originating from C++
> + source even at link-time. */
> +
> +bool
> +odr_or_derived_type_p (const_tree t)
> +{
> + do
> + {
> + if (odr_type_p (t))
> + return true;
> + /* Function type is a tricky one. Basically we can consider it
> + ODR derived if return type or any of the parameters is.
> + We need to check all parameters because LTO streaming merges
> + common types (such as void) and they are not considered ODR then. */
> + if (TREE_CODE (t) == FUNCTION_TYPE)
> + {
> + if (TYPE_METHOD_BASETYPE (t))
> + t = TYPE_METHOD_BASETYPE (t);
> + else
> + {
> + if (TREE_TYPE (t) && odr_or_derived_type_p (TREE_TYPE (t)))
> + return true;
> + for (t = TYPE_ARG_TYPES (t); t; t = TREE_CHAIN (t))
> + if (odr_or_derived_type_p (TREE_VALUE (t)))
> + return true;
> + return false;
> + }
> + }
> + else
> + t = compound_type_base (t);
> + }
> + while (t);
> + return t;
> +}
> +
> /* Compare types T1 and T2 and return true if they are
> equivalent. */
>
> @@ -1577,6 +1642,20 @@ odr_types_equivalent_p (tree t1, tree t2
> return true;
> }
>
> +/* Return true if TYPE1 and TYPE2 are equivalent for One Definition Rule. */
> +
> +bool
> +odr_types_equivalent_p (tree type1, tree type2)
> +{
> + hash_set<type_pair,pair_traits> visited;
> +
> +#ifdef ENABLE_CHECKING
> + gcc_assert (odr_or_derived_type_p (type1) && odr_or_derived_type_p (type2));
> +#endif
> + return odr_types_equivalent_p (type1, type2, false, NULL,
> + &visited);
> +}
> +
> /* TYPE is equivalent to VAL by ODR, but its tree representation differs
> from VAL->type. This may happen in LTO where tree merging did not merge
> all variants of the same type or due to ODR violation.
> @@ -1701,12 +1780,8 @@ add_type_duplicate (odr_type val, tree t
> base_mismatch = true;
> }
> else
> - {
> - hash_set<type_pair,pair_traits> visited;
> - if (!odr_types_equivalent_p (type1, type2, false, NULL,
> - &visited))
> - base_mismatch = true;
> - }
> + if (!odr_types_equivalent_p (type1, type2))
> + base_mismatch = true;
> if (base_mismatch)
> {
> if (!warned && !val->odr_violated)
> Index: testsuite/gfortran.dg/lto/20091028-2_1.c
> ===================================================================
> --- testsuite/gfortran.dg/lto/20091028-2_1.c (revision 222991)
> +++ testsuite/gfortran.dg/lto/20091028-2_1.c (working copy)
> @@ -1,9 +1,9 @@
> extern void *memcpy(void *dest, const void *src, __SIZE_TYPE__ n);
> char *p;
> -int int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> - int * itypesize, int * typesize,
> - int * DataHandle, char * Data,
> - int * Count, int * code)
> +void int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> + int * itypesize, int * typesize,
> + int * DataHandle, char * Data,
> + int * Count, int * code)
> {
> memcpy (typesize, p, sizeof(int)) ;
> memcpy (Data, p, *Count * *typesize) ;
> Index: testsuite/gfortran.dg/lto/pr41576_1.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr41576_1.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr41576_1.f90 (working copy)
> @@ -5,3 +5,8 @@ program test
> if (c/=1 .or. d/=2) call abort
> end program test
>
> +interface
> + subroutine foo()
> + end subroutine
> +end interface
> +
> Index: testsuite/gfortran.dg/lto/pr41521_0.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr41521_0.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr41521_0.f90 (working copy)
> @@ -1,9 +1,9 @@
> ! { dg-lto-do link }
> -! { dg-lto-options {{-g -flto} {-g -O -flto}} }
> +! { dg-lto-options {{-g -flto -Wno-lto-type-mismatch} {-g -O -flto -Wno-lto-type-mismatch}} }
> program species
> integer spk(2)
> real eval(2)
> spk = 2
> call atom(1.1,spk,eval)
> end program
>
> Index: testsuite/gfortran.dg/lto/pr60635_0.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr60635_0.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr60635_0.f90 (working copy)
> @@ -1,4 +1,5 @@
> ! { dg-lto-do link }
> +! { dg-lto-options {{ -Wno-lto-type-mismatch }} }
> program test
> use iso_fortran_env
>
> Index: testsuite/gfortran.dg/lto/pr41576_0.f90
> ===================================================================
> --- testsuite/gfortran.dg/lto/pr41576_0.f90 (revision 222991)
> +++ testsuite/gfortran.dg/lto/pr41576_0.f90 (working copy)
> @@ -1,5 +1,5 @@
> ! { dg-lto-do run }
> -! { dg-lto-options { { -O2 -flto -Werror } } }
> +! { dg-lto-options { { -O2 -flto -Werror -Wno-lto-type-mismatch } } }
>
> subroutine foo
> common /bar/ a, b
> Index: testsuite/gfortran.dg/lto/20091028-1_1.c
> ===================================================================
> --- testsuite/gfortran.dg/lto/20091028-1_1.c (revision 222991)
> +++ testsuite/gfortran.dg/lto/20091028-1_1.c (working copy)
> @@ -1,9 +1,9 @@
> extern void bcopy(const void *, void *, __SIZE_TYPE__ n);
> char *p;
> -int int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> - int * itypesize, int * typesize,
> - int * DataHandle, char * Data,
> - int * Count, int * code)
> +void int_gen_ti_header_c_ (char * hdrbuf, int * hdrbufsize,
> + int * itypesize, int * typesize,
> + int * DataHandle, char * Data,
> + int * Count, int * code)
> {
> bcopy (typesize, p, sizeof(int)) ;
> bcopy (Data, p, *Count * *typesize) ;
> Index: testsuite/gcc.dg/lto/20120723_0.c
> ===================================================================
> --- testsuite/gcc.dg/lto/20120723_0.c (revision 222991)
> +++ testsuite/gcc.dg/lto/20120723_0.c (working copy)
> @@ -3,7 +3,7 @@
> ??? This testcase is invalid C and can only pass on specific platforms. */
> /* { dg-lto-do run } */
> /* { dg-skip-if "" { { sparc*-*-* } && ilp32 } { "*" } { "" } } */
> -/* { dg-lto-options { {-O3 -fno-early-inlining -flto}} } */
> +/* { dg-lto-options { {-O3 -fno-early-inlining -flto -Wno-lto-type-mismatch}} } */
>
> extern void abort (void);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve LTO type checking during symtab merging
2015-05-11 4:45 ` Jan Hubicka
2015-05-12 10:04 ` Richard Biener
@ 2015-05-17 10:42 ` Andreas Schwab
1 sibling, 0 replies; 10+ messages in thread
From: Andreas Schwab @ 2015-05-17 10:42 UTC (permalink / raw)
To: Jan Hubicka; +Cc: Richard Biener, gcc-patches, burnus
Jan Hubicka <hubicka@ucw.cz> writes:
> * gfortran.dg/lto/pr41576_1.f90: Add interface.
> * gfortran.dg/lto/pr41521_0.f90: Disable lto-type-mismatch
FAIL: gfortran.dg/lto/pr41521 f_lto_pr41521_0.o assemble, -g -O -flto -Wno-lto-type-mismatch
FAIL: gfortran.dg/lto/pr41576 f_lto_pr41576_1.o assemble, -O2 -flto -Werror -Wno-lto-type-mismatch
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-05-17 8:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-27 8:34 Improve LTO type checking during symtab merging Jan Hubicka
2015-04-27 8:54 ` Richard Biener
2015-04-28 14:42 ` Jan Hubicka
2015-04-28 2:53 ` Jan Hubicka
2015-04-28 4:07 ` Jan Hubicka
2015-04-28 7:15 ` Tobias Burnus
2015-04-28 8:14 ` Richard Biener
2015-05-11 4:45 ` Jan Hubicka
2015-05-12 10:04 ` Richard Biener
2015-05-17 10:42 ` Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).