Bug 61886 - [4.9/5/6 Regression] LTO breaks fread with _FORTIFY_SOURCE=2
Summary: [4.9/5/6 Regression] LTO breaks fread with _FORTIFY_SOURCE=2
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: lto (show other bugs)
Version: 4.9.1
: P2 normal
Target Milestone: 6.0
Assignee: Jan Hubicka
URL:
Keywords: diagnostic, lto, wrong-code
: 62249 (view as bug list)
Depends on: 25140 68820
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-23 10:40 UTC by Richard Biener
Modified: 2016-01-19 12:58 UTC (History)
11 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.7.4, 6.0
Known to fail: 4.8.3, 4.9.1, 5.3.0
Last reconfirmed: 2014-07-23 00:00:00


Attachments
testcase extracted from cairo test suite (25.77 KB, text/plain)
2014-07-23 10:40 UTC, Richard Biener
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2014-07-23 10:40:03 UTC
Created attachment 33176 [details]
testcase extracted from cairo test suite

With the attached testcase extracted from the Cairo testsuite (which they build with -flto ...) you get

> gcc-4.9 create-for-stream.i -O2 -flto -r -nostdlib
In function ‘__fread_alias’,
    inlined from ‘test_surface’ at create-for-stream.c:218:9:
/usr/include/bits/stdio2.h:290:2: warning: call to ‘__fread_chk_warn’ declared with attribute warning: fread called with bigger size * nmemb than length of destination buffer
  return __fread_chk (__ptr, __bos0 (__ptr), __size, __n, __stream);
  ^

where we mangle compile-time

  <bb 13>:
  _55 = wc.index;
  _77 = __builtin_constant_p (_55);
  if (_77 == 0)
    goto <bb 15>;
  else
    goto <bb 14>;

  <bb 14>:
  _78 = _55 | 1;
  if (_78 > 4294967295)
    goto <bb 15>;
  else
    goto <bb 16>;

  <bb 15>:
  _79 = __fread_chk (&file_contents, 4096, 1, _55, fp_48);
  goto <bb 19>;

  <bb 16>:
  if (_55 > 4096)
    goto <bb 17>;
  else
    goto <bb 18>;

  <bb 17>:
  _81 = *__fread_chk (&file_contents, 4096, 1, _55, fp_48);
  goto <bb 19>;

  <bb 18>:
  _82 = *fread (&file_contents, 1, _55, fp_48);

  <bb 19>:
  # _83 = PHI <_79(15), _81(17), _82(18)>

into the bogus

  <bb 13>:
  _49 = wc.index;
  _64 = __builtin_constant_p (_49);
  if (_64 == 0)
    goto <bb 15>;
  else
    goto <bb 14>;

  <bb 14>:
  _65 = _49 | 1;
  if (_65 > 4294967295)
    goto <bb 15>;
  else
    goto <bb 16>;

  <bb 15>:
  _66 = __fread_chk_warn (&file_contents, 4096, 1, _49, fp_43);
  goto <bb 19>;

  <bb 16>:
  if (_49 > 4096)
    goto <bb 17>;
  else
    goto <bb 18>;

  <bb 17>:
  _67 = __fread_chk_warn (&file_contents, 4096, 1, _49, fp_43);
  goto <bb 19>;

  <bb 18>:
  _68 = __fread_alias (&file_contents, 1, _49, fp_43);

  <bb 19>:
  # _69 = PHI <_66(15), _67(17), _68(18)>

somehow messing up the aliases game glibc plays:

extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen,
                           size_t __size, size_t __n,
                           FILE *__restrict __stream) __wur;
extern size_t __REDIRECT (__fread_alias,
                          (void *__restrict __ptr, size_t __size,
                           size_t __n, FILE *__restrict __stream),
                          fread) __wur;
extern size_t __REDIRECT (__fread_chk_warn,
                          (void *__restrict __ptr, size_t __ptrlen,
                           size_t __size, size_t __n,
                           FILE *__restrict __stream),
                          __fread_chk)
     __wur __warnattr ("fread called with bigger size * nmemb than length "
                       "of destination buffer");

__fortify_function __wur size_t
fread (void *__restrict __ptr, size_t __size, size_t __n,
       FILE *__restrict __stream)
{
  if (__bos0 (__ptr) != (size_t) -1)
    {
      if (!__builtin_constant_p (__size)
          || !__builtin_constant_p (__n)
          || (__size | __n) >= (((size_t) 1) << (8 * sizeof (size_t) / 2)))
        return __fread_chk (__ptr, __bos0 (__ptr), __size, __n, __stream);

      if (__size * __n > __bos0 (__ptr))
        return __fread_chk_warn (__ptr, __bos0 (__ptr), __size, __n, __stream);
    }
  return __fread_alias (__ptr, __size, __n, __stream);
}

Merging nodes for *__fread_chk. Candidates:
*__fread_chk/98 (__fread_chk_warn) @0x7ffff6dc2b80
  Type: function
  Visibility: undef external public
  next sharing asm name: 97
  References:
  Referring:
  Read from file: /tmp/ccu88pge.o
  First run: 0
  Function flags:
  Called by: test_surface/78
  Calls:
__fread_chk/97 (__fread_chk) @0x7ffff6dc2cf0
  Type: function
  Visibility: external public
  previous sharing asm name: 98
  References:
  Referring:
  Read from file: /tmp/ccu88pge.o
  First run: 0
  Function flags:
  Called by: test_surface/78 (0.00 per call)
After resolution:
*__fread_chk/98 (__fread_chk_warn) @0x7ffff6dc2b80
  Type: function
  Visibility: undef external public
  next sharing asm name: 97
  References:
  Referring:
  Read from file: /tmp/ccu88pge.o
  First run: 0
  Function flags:
  Called by: test_surface/78
  Calls:
__fread_chk/97 (__fread_chk) @0x7ffff6dc2cf0
  Type: function
  Visibility: external public
  previous sharing asm name: 98
  References:
  Referring:
  Read from file: /tmp/ccu88pge.o
  First run: 0
  Function flags:
  Called by: test_surface/78 (0.00 per call)
  Calls:

but we obviously shouldn't merge these ...

(I believe we shouldn't drop any aliases at LTO symbol merging time, which
means _not_ merging based on asm-name?)
Comment 1 Richard Biener 2014-07-23 10:58:03 UTC
Btw, it seems that at the point we merge in lto-symtab.c the cgraph nodes are
not yet populated.  Neither of the two candidates are marked as alias.

So maybe the wrong thing already happens earlier, during compile-time
and we shouldn't do any symtab merging?  at least from symbols originating
from the same TU?  That is, tree merging already should catch most
true equivalencies and cgraph merging shouldn't be symtab-driven?

That said, I wonder how to fix things up properly.

The following fixes the testcase, not merging the actual symtab intra-TU
and retaining decls that have a symtab entry recorded.

Index: gcc/lto/lto-symtab.c
===================================================================
--- gcc/lto/lto-symtab.c        (revision 212927)
+++ gcc/lto/lto-symtab.c        (working copy)
@@ -575,6 +575,9 @@ lto_symtab_merge_symbols_1 (symtab_node

       if (!lto_symtab_symbol_p (e))
        continue;
+      /* Do not merge symtab nodes originating from the same TU.  */
+      if (e->lto_file_data == prevailing->lto_file_data)
+       continue;
       cgraph_node *ce = dyn_cast <cgraph_node *> (e);
       if (ce && !DECL_BUILT_IN (e->decl))
        lto_cgraph_replace_node (ce, cgraph (prevailing));
@@ -680,6 +683,12 @@ lto_symtab_prevailing_decl (tree decl)
   if (TREE_CODE (decl) == FUNCTION_DECL && DECL_BUILT_IN (decl))
     return decl;

+  /* If the decl retained its symtab node then it prevails.  This
+     preserves semantically different decls for the same underlying
+     symbol, like aliases that have been resolved at compile-time.  */
+  if (symtab_get_node (decl))
+    return decl;
+
   /* Ensure DECL_ASSEMBLER_NAME will not set assembler name.  */
   gcc_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
Comment 2 Richard Biener 2014-07-23 11:13:05 UTC
I'm testing the patch, testcase:

/* { dg-lto-do link } */
/* { dg-lto-options { { -flto -O2 -Werror } } } */

typedef __SIZE_TYPE__ size_t;
typedef struct _IO_FILE FILE;

extern size_t __fread_chk_warn (void *__restrict __ptr, size_t __ptrlen, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "__fread_chk")      __attribute__ ((__warn_unused_result__)) __attribute__((__warning__ ("fread called with bigger size * nmemb than length " "of destination buffer")));

extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) __attribute__ ((__artificial__)) __attribute__ ((__warn_unused_result__))
size_t
fread (void *__restrict __ptr, size_t __size, size_t __n,
       FILE *__restrict __stream)
{
  if (__builtin_object_size (__ptr, 0) != (size_t) -1)
    {
      if (!__builtin_constant_p (__size)
          || !__builtin_constant_p (__n)
          || (__size | __n) >= (((size_t) 1) << (8 * sizeof (size_t) / 2)))
        return __fread_chk (__ptr, __builtin_object_size (__ptr, 0), __size, __n, __stream);
      if (__size * __n > __builtin_object_size (__ptr, 0))
        return __fread_chk_warn (__ptr, __builtin_object_size (__ptr, 0), __size, __n, __stream);
    }
}

volatile size_t nmemb;
FILE *fp;
int main ()
{
  char file_contents[4096];
  /* We shouldn't get this resolved to a call to __fread_chk_warn.  */
  return fread (file_contents, 1, nmemb, fp);
}
Comment 3 Richard Biener 2014-07-23 11:57:58 UTC
Fails LTO bootstrap with

/abuild/rguenther/obj/./prev-gcc/xg++ -B/abuild/rguenther/obj/./prev-gcc/ -B/usr/local/x86_64-unknown-linux-gnu/bin/ -nostdinc++ -B/abuild/rguenther/obj/prev-x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs -B/abuild/rguenther/obj/prev-x86_64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs  -I/abuild/rguenther/obj/prev-x86_64-unknown-linux-gnu/libstdc++-v3/include/x86_64-unknown-linux-gnu  -I/abuild/rguenther/obj/prev-x86_64-unknown-linux-gnu/libstdc++-v3/include  -I/space/rguenther/src/svn/trunk/libstdc++-v3/libsupc++ -L/abuild/rguenther/obj/prev-x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs -L/abuild/rguenther/obj/prev-x86_64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs   -g -O2 -flto=jobserver -frandom-seed=1 -ffat-lto-objects -DIN_GCC    -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common  -DHAVE_CONFIG_H -DGENERATOR_FILE -static-libstdc++ -static-libgcc  -o build/genhooks \
            build/genhooks.o build/errors.o .././libiberty/libiberty.a
lto1: internal compiler error: in maybe_add_reference, at symtab.c:613
0x68a129 symtab_node::maybe_add_reference(tree_node*, ipa_ref_use, gimple_statement_base*)
        /space/rguenther/src/svn/trunk/gcc/symtab.c:613
0x6a16de cgraph_create_virtual_clone(cgraph_node*, vec<cgraph_edge*, va_heap, vl_ptr>, vec<ipa_replace_map*, va_gc, vl_embed>*, bitmap_head*, char const*)
        /space/rguenther/src/svn/trunk/gcc/cgraphclones.c:582
0x12ca0e5 create_specialized_node
        /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:2802
0x12ce757 decide_whether_version_node
        /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:3556
0x12ce8d3 ipcp_decision_stage
        /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:3668
0x12cef4d ipcp_driver
        /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:3714
0x12cefba execute
        /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:3805
Comment 4 Richard Biener 2014-07-23 13:08:31 UTC
Fully preprocessed glibc fread stuff (reduced testcase drops main path
and a prototype for __fread_chk):

extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen,
      size_t __size, size_t __n,
      FILE *__restrict __stream) __attribute__ ((__warn_unused_result__));
extern size_t __fread_alias (void *__restrict __ptr, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "fread")


            __attribute__ ((__warn_unused_result__));
extern size_t __fread_chk_warn (void *__restrict __ptr, size_t __ptrlen, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "__fread_chk")




     __attribute__ ((__warn_unused_result__)) __attribute__((__warning__ ("fread called with bigger size * nmemb than length " "of destination buffer")))
                                 ;

extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) __attribute__ ((__artificial__)) __attribute__ ((__warn_unused_result__)) size_t
fread (void *__restrict __ptr, size_t __size, size_t __n,
       FILE *__restrict __stream)
{
  if (__builtin_object_size (__ptr, 0) != (size_t) -1)
    {
      if (!__builtin_constant_p (__size)
   || !__builtin_constant_p (__n)
   || (__size | __n) >= (((size_t) 1) << (8 * sizeof (size_t) / 2)))
 return __fread_chk (__ptr, __builtin_object_size (__ptr, 0), __size, __n, __stream);

      if (__size * __n > __builtin_object_size (__ptr, 0))
 return __fread_chk_warn (__ptr, __builtin_object_size (__ptr, 0), __size, __n, __stream);
    }
  return __fread_alias (__ptr, __size, __n, __stream);
}
Comment 5 Jan Hubicka 2014-07-23 13:37:43 UTC
This is a difficult problem.  In early cgraph days GCC code was converted to assume that there are never duplicated declarations of a given symbol. This is precisely what glibc does to keep these duplicates flow through the compilation.
It is a long standing bug that we have such duplicate declarations and that we do not reject these as an error or fix internaly.

A clear fix would be to make alias from glibc SO for fread_chk and fread_chk_warn that can be used for those two different calls.  But without changling glibc's API we need to work out how to introduce the alias internally.  We support similar bookeeping for wearkrefs that are "syntactic" aliases resulting in the same assembler name as their target.  I suppose we can do the same here, but it is ugly - I would much preffer those hacks to not exist at all.

I will try to prepare patch and see how contained it is.

Honza
Comment 6 Richard Biener 2014-08-27 09:59:56 UTC
Honza, did you get anything working?
Comment 7 Jan Hubicka 2014-08-27 14:51:19 UTC
> Honza, did you get anything working?
Sorry, I am at Mountain trip till 6th of September, so I do not have much
chance to hack.  Will take it as priority afterwards.  I think cleanest
approach would be to lower these calls into sequence of warning/error builtin
and call.
Comment 8 rguenther@suse.de 2014-08-28 08:20:04 UTC
On Wed, 27 Aug 2014, hubicka at ucw dot cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> 
> --- Comment #7 from Jan Hubicka <hubicka at ucw dot cz> ---
> > Honza, did you get anything working?
> Sorry, I am at Mountain trip till 6th of September, so I do not have much
> chance to hack.  Will take it as priority afterwards.  I think cleanest
> approach would be to lower these calls into sequence of warning/error builtin
> and call.

The issue is that we resolve aliases in a bogus way, not warning/error
stuff.
Comment 9 Jan Hubicka 2014-09-08 00:42:15 UTC
> The issue is that we resolve aliases in a bogus way, not warning/error
> stuff.

Those are not aliases, but duplicated declarations that are supposed to not
exist since one declaration rule was introduced in early 4.x series.
It is bug that frotnend allowed creating them and made it part of the fortify
interface. I still think it is better to resolve those early and start rejecting
any other uses than the fortify case...

Honza
Comment 10 rguenther@suse.de 2014-09-08 07:52:54 UTC
On Mon, 8 Sep 2014, hubicka at ucw dot cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> 
> --- Comment #9 from Jan Hubicka <hubicka at ucw dot cz> ---
> > The issue is that we resolve aliases in a bogus way, not warning/error
> > stuff.
> 
> Those are not aliases, but duplicated declarations that are supposed to not
> exist since one declaration rule was introduced in early 4.x series.
> It is bug that frotnend allowed creating them and made it part of the fortify
> interface. I still think it is better to resolve those early and start
> rejecting
> any other uses than the fortify case...

Well - whatever makes the fortify case work as expected works for me.
Note that it would be nice to fix this on the 4.9 branch as well.
Comment 11 Jan Hubicka 2014-10-06 20:22:57 UTC
Hi,
this patch implements the lowring.  Each call with warn attribute triggers code
in cgraphunit that inserts call to bulitin_warning/error that is output at
expansion time.

Do we have way to define bulitin that is not user accessible?

Also we do not have way to define LOOPING_CONST bulitin, so I am simply forcing
the flag in cgraphunit.c that is somewhat ugly.

One of consequences of this approach is that indirect calls to functions with
warn attributes will not produce warning.  I think it is sort of acceptable
because we also do not warn when the indirect call is produced late by RTL backend.

Honza

Index: expr.c
===================================================================
--- expr.c	(revision 215901)
+++ expr.c	(working copy)
@@ -10346,21 +10346,7 @@ expand_expr_real_1 (tree exp, rtx target
       if (CALL_EXPR_VA_ARG_PACK (exp))
 	error ("%Kinvalid use of %<__builtin_va_arg_pack ()%>", exp);
       {
-	tree fndecl = get_callee_fndecl (exp), attr;
-
-	if (fndecl
-	    && (attr = lookup_attribute ("error",
-					 DECL_ATTRIBUTES (fndecl))) != NULL)
-	  error ("%Kcall to %qs declared with attribute error: %s",
-		 exp, identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 1)),
-		 TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
-	if (fndecl
-	    && (attr = lookup_attribute ("warning",
-					 DECL_ATTRIBUTES (fndecl))) != NULL)
-	  warning_at (tree_nonartificial_location (exp),
-		      0, "%Kcall to %qs declared with attribute warning: %s",
-		      exp, identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 1)),
-		      TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
+	tree fndecl = get_callee_fndecl (exp);
 
 	/* Check for a built-in function.  */
 	if (fndecl && DECL_BUILT_IN (fndecl))
Index: builtin-types.def
===================================================================
--- builtin-types.def	(revision 215901)
+++ builtin-types.def	(working copy)
@@ -581,3 +581,5 @@ DEF_FUNCTION_TYPE_2 (BT_FN_VOID_VPTR_LDO
 		     BT_VOLATILE_PTR, BT_LONGDOUBLE)
 DEF_FUNCTION_TYPE_2 (BT_FN_VOID_VPTR_SIZE, BT_VOID,
 		     BT_VOLATILE_PTR, BT_SIZE)
+DEF_FUNCTION_TYPE_2 (BT_FN_VOID_CONST_STRING_CONST_STRING,
+		     BT_VOID, BT_CONST_STRING, BT_CONST_STRING)
Index: builtins.c
===================================================================
--- builtins.c	(revision 215901)
+++ builtins.c	(working copy)
@@ -59,6 +59,8 @@ along with GCC; see the file COPYING3.
 #include "builtins.h"
 #include "ubsan.h"
 #include "cilk.h"
+#include "pretty-print.h"
+#include "print-tree.h"
 
 
 static tree do_mpc_arg1 (tree, tree, int (*)(mpc_ptr, mpc_srcptr, mpc_rnd_t));
@@ -6816,6 +6818,31 @@ expand_builtin (tree exp, rtx target, rt
       expand_builtin_cilk_pop_frame (exp);
       return const0_rtx;
 
+    case BUILT_IN_WARNING:
+      const char * arg0;
+      const char * arg1;
+      if (!validate_arglist (exp, POINTER_TYPE, POINTER_TYPE, VOID_TYPE)
+	  || (arg0 = c_getstr (CALL_EXPR_ARG (exp, 0))) == NULL
+	  || (arg1 = c_getstr (CALL_EXPR_ARG (exp, 1))) == NULL)
+	warning_at (tree_nonartificial_location (exp), 0,
+		    "%K__builtin_warning used without both arguments being string constants",
+		    exp);
+      else
+	warning_at (tree_nonartificial_location (exp),
+		    0, "%Kcall to %qs declared with attribute warning: %s",
+		    exp, arg0, identifier_to_locale (arg1));
+      return const0_rtx;
+    case BUILT_IN_ERROR:
+      if (!validate_arglist (exp, POINTER_TYPE, POINTER_TYPE, VOID_TYPE)
+	  || (arg0 = c_getstr (CALL_EXPR_ARG (exp, 0))) == NULL
+	  || (arg1 = c_getstr (CALL_EXPR_ARG (exp, 1))) == NULL)
+	error ("%K__builtin_error used without both arguments being string constants",
+	       exp);
+      else
+	error ("%Kcall to %qs declared with attribute warning: %s",
+	       exp, arg0, identifier_to_locale (arg1));
+      return const0_rtx;
+
     default:	/* just do library call, if unknown builtin */
       break;
     }
Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 215901)
+++ cgraphunit.c	(working copy)
@@ -211,6 +211,7 @@ along with GCC; see the file COPYING3.
 #include "tree-nested.h"
 #include "gimplify.h"
 #include "dbgcnt.h"
+#include "expr.h"
 
 /* Queue of cgraph nodes scheduled to be added into cgraph.  This is a
    secondary queue used during optimization to accommodate passes that
@@ -976,8 +977,30 @@ analyze_functions (void)
 		cnode->analyze ();
 
 	      for (edge = cnode->callees; edge; edge = edge->next_callee)
-		if (edge->callee->definition)
-		   enqueue_node (edge->callee);
+		{
+		  tree attr, err_attr = NULL;
+		  if (edge->callee->definition)
+		     enqueue_node (edge->callee);
+		  if ((attr = lookup_attribute ("warning",
+					        DECL_ATTRIBUTES (edge->callee->decl))) != NULL
+		      || (err_attr = lookup_attribute ("warning",
+							DECL_ATTRIBUTES (edge->callee->decl))))
+		    {
+		      gimple_stmt_iterator gsi = gsi_for_stmt (edge->call_stmt);
+		      const char *arg0 = lang_hooks.decl_printable_name (edge->callee->decl, 1);
+		      const char *arg1= TREE_STRING_POINTER
+					 (TREE_VALUE (TREE_VALUE (attr ? attr : err_attr)));
+		      tree dest = builtin_decl_implicit (attr ? BUILT_IN_WARNING : BUILT_IN_ERROR);
+
+		      cgraph_node::get_create (dest)->set_const_flag (true, true);
+		      gimple call = gimple_build_call (dest, 2,
+						       build_string_literal (strlen (arg0), arg0),
+						       build_string_literal (strlen (arg1), arg1));
+		      gsi_insert_before (&gsi, call, GSI_SAME_STMT);
+		      cnode->create_edge (cgraph_node::get_create (dest), call,
+					  edge->count, edge->frequency);
+		    }
+		}
 	      if (optimize && flag_devirtualize)
 		{
 		  cgraph_edge *next;
Index: builtins.def
===================================================================
--- builtins.def	(revision 215901)
+++ builtins.def	(working copy)
@@ -820,6 +820,9 @@ DEF_EXT_LIB_BUILTIN    (BUILT_IN_PRINTF_
 DEF_EXT_LIB_BUILTIN    (BUILT_IN_VFPRINTF_CHK, "__vfprintf_chk", BT_FN_INT_FILEPTR_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_3_0)
 DEF_EXT_LIB_BUILTIN    (BUILT_IN_VPRINTF_CHK, "__vprintf_chk", BT_FN_INT_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_2_0)
 
+DEF_GCC_BUILTIN        (BUILT_IN_ERROR, "error", BT_FN_VOID_CONST_STRING_CONST_STRING, ATTR_NOTHROW_LEAF_LIST)
+DEF_GCC_BUILTIN        (BUILT_IN_WARNING, "warning", BT_FN_VOID_CONST_STRING_CONST_STRING, ATTR_NOTHROW_LEAF_LIST)
+
 /* Profiling hooks.  */
 DEF_BUILTIN (BUILT_IN_PROFILE_FUNC_ENTER, "__cyg_profile_func_enter", BUILT_IN_NORMAL, BT_FN_VOID_PTR_PTR, BT_LAST,
 	     false, false, false, ATTR_NULL, true, true)
Comment 12 Jakub Jelinek 2014-10-06 20:34:24 UTC
On Mon, Oct 06, 2014 at 10:22:21PM +0200, Jan Hubicka wrote:
> this patch implements the lowring.  Each call with warn attribute triggers code
> in cgraphunit that inserts call to bulitin_warning/error that is output at
> expansion time.
> 
> Do we have way to define bulitin that is not user accessible?

internal-fn* builtins are not user accessible.

	Jakub
Comment 13 Jan Hubicka 2014-10-06 21:55:59 UTC
Hi,
I am testing this variant of the patch.
For gcc-4.9 branch it may make sense to enable the new patch for LTO only.

Index: internal-fn.c
===================================================================
--- internal-fn.c	(revision 215958)
+++ internal-fn.c	(working copy)
@@ -37,6 +37,8 @@ along with GCC; see the file COPYING3.
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "diagnostic-core.h"
+#include "builtins.h"
+#include "pretty-print.h"
 
 /* The names of each internal function, indexed by function number.  */
 const char *const internal_fn_name_array[] = {
@@ -915,6 +917,26 @@ expand_BUILTIN_EXPECT (gimple stmt)
     emit_move_insn (target, val);
 }
 
+static void
+expand_OUTPUT_ERROR (gimple stmt)
+{
+  const char * arg0 = c_getstr (gimple_call_arg (stmt, 0));
+  const char * arg1 = c_getstr (gimple_call_arg (stmt, 1));
+  error_at (gimple_location (stmt),
+	    "Call to %qs declared with attribute error: %s",
+	    arg0, identifier_to_locale (arg1));
+}
+
+static void
+expand_OUTPUT_WARNING (gimple stmt)
+{
+  const char * arg0 = c_getstr (gimple_call_arg (stmt, 0));
+  const char * arg1 = c_getstr (gimple_call_arg (stmt, 1));
+  warning_at (gimple_location (stmt),
+	      0, "call to %qs declared with attribute warning: %s",
+	      arg0, identifier_to_locale (arg1));
+}
+
 /* Routines to expand each internal function, indexed by function number.
    Each routine has the prototype:
 
Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 215958)
+++ cgraphunit.c	(working copy)
@@ -211,6 +211,8 @@ along with GCC; see the file COPYING3.
 #include "tree-nested.h"
 #include "gimplify.h"
 #include "dbgcnt.h"
+#include "expr.h"
+#include "internal-fn.h"
 
 /* Queue of cgraph nodes scheduled to be added into cgraph.  This is a
    secondary queue used during optimization to accommodate passes that
@@ -976,8 +978,29 @@ analyze_functions (void)
 		cnode->analyze ();
 
 	      for (edge = cnode->callees; edge; edge = edge->next_callee)
-		if (edge->callee->definition)
-		   enqueue_node (edge->callee);
+		{
+		  tree attr, err_attr = NULL;
+		  if (edge->callee->definition)
+		     enqueue_node (edge->callee);
+		  if ((attr = lookup_attribute ("warning",
+					        DECL_ATTRIBUTES (edge->callee->decl))) != NULL
+		      || (err_attr = lookup_attribute ("warning",
+							DECL_ATTRIBUTES (edge->callee->decl))))
+		    {
+		      gimple_stmt_iterator gsi = gsi_for_stmt (edge->call_stmt);
+		      const char *arg0 = lang_hooks.decl_printable_name (edge->callee->decl, 1);
+		      const char *arg1= TREE_STRING_POINTER
+					 (TREE_VALUE (TREE_VALUE (attr ? attr : err_attr)));
+
+		      gimple call = gimple_build_call_internal
+				      (attr ? IFN_OUTPUT_WARNING : IFN_OUTPUT_ERROR, 2,
+				       build_string_literal (strlen (arg0), arg0),
+				       build_string_literal (strlen (arg1), arg1));
+		      gsi_insert_before (&gsi, call, GSI_SAME_STMT);
+		      gimple_set_location (call, gimple_location (edge->call_stmt));
+		      gimple_set_block (call, gimple_block (edge->call_stmt));
+		    }
+		}
 	      if (optimize && flag_devirtualize)
 		{
 		  cgraph_edge *next;
Index: expr.c
===================================================================
--- expr.c	(revision 215958)
+++ expr.c	(working copy)
@@ -10346,21 +10346,7 @@ expand_expr_real_1 (tree exp, rtx target
       if (CALL_EXPR_VA_ARG_PACK (exp))
 	error ("%Kinvalid use of %<__builtin_va_arg_pack ()%>", exp);
       {
-	tree fndecl = get_callee_fndecl (exp), attr;
-
-	if (fndecl
-	    && (attr = lookup_attribute ("error",
-					 DECL_ATTRIBUTES (fndecl))) != NULL)
-	  error ("%Kcall to %qs declared with attribute error: %s",
-		 exp, identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 1)),
-		 TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
-	if (fndecl
-	    && (attr = lookup_attribute ("warning",
-					 DECL_ATTRIBUTES (fndecl))) != NULL)
-	  warning_at (tree_nonartificial_location (exp),
-		      0, "%Kcall to %qs declared with attribute warning: %s",
-		      exp, identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 1)),
-		      TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
+	tree fndecl = get_callee_fndecl (exp);
 
 	/* Check for a built-in function.  */
 	if (fndecl && DECL_BUILT_IN (fndecl))
Index: internal-fn.def
===================================================================
--- internal-fn.def	(revision 215958)
+++ internal-fn.def	(working copy)
@@ -56,3 +56,5 @@ DEF_INTERNAL_FN (UBSAN_CHECK_MUL, ECF_CO
 DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
 DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".W...")
+DEF_INTERNAL_FN (OUTPUT_ERROR, ECF_TM_PURE | ECF_LEAF | ECF_CONST | ECF_NOTHROW | ECF_LOOPING_CONST_OR_PURE, NULL)
+DEF_INTERNAL_FN (OUTPUT_WARNING, ECF_TM_PURE | ECF_LEAF | ECF_CONST | ECF_NOTHROW | ECF_LOOPING_CONST_OR_PURE, NULL)
Comment 14 Jakub Jelinek 2014-10-06 22:08:21 UTC
On Mon, Oct 06, 2014 at 11:55:23PM +0200, Jan Hubicka wrote:
> Hi,
> I am testing this variant of the patch.
> For gcc-4.9 branch it may make sense to enable the new patch for LTO only.

Not printing the inlining backtrace would be IMHO a significant regression.

	Jakub
Comment 15 Jan Hubicka 2014-10-06 22:19:01 UTC
> On Mon, Oct 06, 2014 at 11:55:23PM +0200, Jan Hubicka wrote:
> > Hi,
> > I am testing this variant of the patch.
> > For gcc-4.9 branch it may make sense to enable the new patch for LTO only.
> 
> Not printing the inlining backtrace would be IMHO a significant regression.

OK, how do I print it?  I keep the BLOCK of the original expresison, so it is there.

Honza
> 
> 	Jakub
Comment 16 Jakub Jelinek 2014-10-06 22:38:48 UTC
On Tue, Oct 07, 2014 at 12:18:24AM +0200, Jan Hubicka wrote:
> > On Mon, Oct 06, 2014 at 11:55:23PM +0200, Jan Hubicka wrote:
> > > Hi,
> > > I am testing this variant of the patch.
> > > For gcc-4.9 branch it may make sense to enable the new patch for LTO only.
> > 
> > Not printing the inlining backtrace would be IMHO a significant regression.
> 
> OK, how do I print it?  I keep the BLOCK of the original expresison, so it is there.

%K in the format string, assuming the call has locus with the right block,
should do that.  At least with -g, without -g or with LTO it will be less
accurate.

	Jakub
Comment 17 Jan Hubicka 2014-10-06 22:44:16 UTC
> 
> %K in the format string, assuming the call has locus with the right block,
> should do that.  At least with -g, without -g or with LTO it will be less
> accurate.

Yep, for that I need a tree to pass in. Do I need to build something or is there
way to pass in gimple statmeent?

Honza
Comment 18 Jan Hubicka 2014-10-07 05:46:36 UTC
Hi,
actually I can just add the location to the first argument to avoid the need
to build extra tree...

Somewhat ugly, but seems to work.

Index: internal-fn.c
===================================================================
--- internal-fn.c	(revision 215958)
+++ internal-fn.c	(working copy)
@@ -37,6 +37,8 @@ along with GCC; see the file COPYING3.
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "diagnostic-core.h"
+#include "builtins.h"
+#include "pretty-print.h"
 
 /* The names of each internal function, indexed by function number.  */
 const char *const internal_fn_name_array[] = {
@@ -915,6 +917,28 @@ expand_BUILTIN_EXPECT (gimple stmt)
     emit_move_insn (target, val);
 }
 
+static void
+expand_OUTPUT_ERROR (gimple stmt)
+{
+  const char * arg0 = c_getstr (gimple_call_arg (stmt, 0));
+  const char * arg1 = c_getstr (gimple_call_arg (stmt, 1));
+  error_at (gimple_location (stmt),
+	    "%KCall to %qs declared with attribute error: %s",
+	    gimple_call_arg (stmt, 0),
+	    arg0, identifier_to_locale (arg1));
+}
+
+static void
+expand_OUTPUT_WARNING (gimple stmt)
+{
+  const char * arg0 = c_getstr (gimple_call_arg (stmt, 0));
+  const char * arg1 = c_getstr (gimple_call_arg (stmt, 1));
+  warning_at (gimple_location (stmt),
+	      0, "%Kcall to %qs declared with attribute warning: %s",
+	      gimple_call_arg (stmt, 0),
+	      arg0, identifier_to_locale (arg1));
+}
+
 /* Routines to expand each internal function, indexed by function number.
    Each routine has the prototype:
 
Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 215958)
+++ cgraphunit.c	(working copy)
@@ -211,6 +211,8 @@ along with GCC; see the file COPYING3.
 #include "tree-nested.h"
 #include "gimplify.h"
 #include "dbgcnt.h"
+#include "expr.h"
+#include "internal-fn.h"
 
 /* Queue of cgraph nodes scheduled to be added into cgraph.  This is a
    secondary queue used during optimization to accommodate passes that
@@ -976,8 +978,33 @@ analyze_functions (void)
 		cnode->analyze ();
 
 	      for (edge = cnode->callees; edge; edge = edge->next_callee)
-		if (edge->callee->definition)
-		   enqueue_node (edge->callee);
+		{
+		  tree attr, err_attr = NULL;
+		  if (edge->callee->definition)
+		     enqueue_node (edge->callee);
+		  if ((attr = lookup_attribute ("warning",
+					        DECL_ATTRIBUTES (edge->callee->decl))) != NULL
+		      || (err_attr = lookup_attribute ("warning",
+							DECL_ATTRIBUTES (edge->callee->decl))))
+		    {
+		      gimple_stmt_iterator gsi = gsi_for_stmt (edge->call_stmt);
+		      const char *arg0 = lang_hooks.decl_printable_name (edge->callee->decl, 1);
+		      const char *arg1= TREE_STRING_POINTER
+					 (TREE_VALUE (TREE_VALUE (attr ? attr : err_attr)));
+		      tree arg0_expr = build_string_literal (strlen (arg0), arg0);
+
+		      gimple call = gimple_build_call_internal
+				      (attr ? IFN_OUTPUT_WARNING : IFN_OUTPUT_ERROR, 2,
+				       arg0_expr,
+				       build_string_literal (strlen (arg1), arg1));
+		      gsi_insert_before (&gsi, call, GSI_SAME_STMT);
+		      gimple_set_location (call, gimple_location (edge->call_stmt));
+		      gimple_set_block (call, gimple_block (edge->call_stmt));
+		      /* Disgnostic code needs tree to pick inline stack from. */
+		      SET_EXPR_LOCATION (arg0_expr, gimple_location (edge->call_stmt));
+		      TREE_SET_BLOCK (arg0_expr, gimple_block (edge->call_stmt));
+		    }
+		}
 	      if (optimize && flag_devirtualize)
 		{
 		  cgraph_edge *next;
Index: builtin-types.def
===================================================================
--- builtin-types.def	(revision 215958)
+++ builtin-types.def	(working copy)
@@ -581,3 +581,5 @@ DEF_FUNCTION_TYPE_2 (BT_FN_VOID_VPTR_LDO
 		     BT_VOLATILE_PTR, BT_LONGDOUBLE)
 DEF_FUNCTION_TYPE_2 (BT_FN_VOID_VPTR_SIZE, BT_VOID,
 		     BT_VOLATILE_PTR, BT_SIZE)
+DEF_FUNCTION_TYPE_2 (BT_FN_VOID_CONST_STRING_CONST_STRING,
+		     BT_VOID, BT_CONST_STRING, BT_CONST_STRING)
Index: expr.c
===================================================================
--- expr.c	(revision 215958)
+++ expr.c	(working copy)
@@ -10346,21 +10346,7 @@ expand_expr_real_1 (tree exp, rtx target
       if (CALL_EXPR_VA_ARG_PACK (exp))
 	error ("%Kinvalid use of %<__builtin_va_arg_pack ()%>", exp);
       {
-	tree fndecl = get_callee_fndecl (exp), attr;
-
-	if (fndecl
-	    && (attr = lookup_attribute ("error",
-					 DECL_ATTRIBUTES (fndecl))) != NULL)
-	  error ("%Kcall to %qs declared with attribute error: %s",
-		 exp, identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 1)),
-		 TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
-	if (fndecl
-	    && (attr = lookup_attribute ("warning",
-					 DECL_ATTRIBUTES (fndecl))) != NULL)
-	  warning_at (tree_nonartificial_location (exp),
-		      0, "%Kcall to %qs declared with attribute warning: %s",
-		      exp, identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 1)),
-		      TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
+	tree fndecl = get_callee_fndecl (exp);
 
 	/* Check for a built-in function.  */
 	if (fndecl && DECL_BUILT_IN (fndecl))
Comment 19 rguenther@suse.de 2014-10-07 07:23:18 UTC
On Mon, 6 Oct 2014, hubicka at ucw dot cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> 
> --- Comment #11 from Jan Hubicka <hubicka at ucw dot cz> ---
> Hi,
> this patch implements the lowring.  Each call with warn attribute triggers code
> in cgraphunit that inserts call to bulitin_warning/error that is output at
> expansion time.
> 
> Do we have way to define bulitin that is not user accessible?
> 
> Also we do not have way to define LOOPING_CONST bulitin, so I am simply forcing
> the flag in cgraphunit.c that is somewhat ugly.

But this also means that when attaching the attribute to pure/const
functions which result becomes unused and thus the call gets DCEd
will still emit the warning/error.  Similar if the function gets
inlined (formerly no warning/error).

I think you need an alternate lowering for the first case at least,
like passing through the return value.  Not sure how to deal with the 2nd
case.

That said, I don't think this is the way to go to implement the
attribute.
Comment 20 Jakub Jelinek 2014-10-07 09:24:03 UTC
The builtin-types.def part is unnecessary, I don't see internal-fn.def part.
Also, we might need to tune optimizations across the two internal calls (from aliasing POV at least), we certainly want them to act as they have unspecified other effects (i.e. we don't want DCE to delete them, or hoist them before condition guarding them, on the other side, it would be nice if alias would know they don't clobber nor read any memory, are leaf etc.).
Comment 21 Jakub Jelinek 2014-10-07 09:35:02 UTC
The only duplicate decls are the C extern inline __attribute__((gnu_inline))
(or -std=c89/gnu89 extern inline) or C++ inline __attribute__((gnu_inline)).
We do have a middle-end representation of those, don't we?
Do you have a problem that they have the same asm names, or DECL_NAME, something else?
If you wanted different asm names (e.g. normal asm name plus space at the end), we'd need some code to change calls to the functions with space after it back into ones without it if inlining failed.
Comment 22 Jan Hubicka 2014-10-07 19:49:01 UTC
Concerning Richard's comment, it is true that we will produce more warings then
before in case function is optimized out.  But we always did produce extra warnings
when the function call is optimized out (as dead or pure/const unused) during
RTL optimization. Of course this was more common before we got tree-ssa, but the
feature AFAIK predates that.

This is ugly area, because it exposes too much of internals.

> The only duplicate decls are the C extern inline __attribute__((gnu_inline))
> (or -std=c89/gnu89 extern inline) or C++ inline __attribute__((gnu_inline)).
> We do have a middle-end representation of those, don't we?

We don't.  We really replace inline version by offline and mark it noinline.
With Winline you get warning "redefined extern inline functions are not considered for inlining"
Here I think a way around would be to make C/C++ FEs to produce a static inline function
and record in its cgraph node that it should be used for inlining of some other symbol.
Unreachable code removal would need to be extended to deal with this (i.e. not remove it until
references to the real symbol disappears) and inliner can handle it easily redirecting
to the inline version prior inlining.

I never got past implementing the frontend part though.

> Do you have a problem that they have the same asm names, or DECL_NAME,
> something else?

Asm name, since Zack's one declaration changes, we are supposed to have only one
decl for an ASM name.  It is not always true - the warning attribute is one offender
and in some cases FEs still break the rule.

> If you wanted different asm names (e.g. normal asm name plus space at the end),
> we'd need some code to change calls to the functions with space after it back
> into ones without it if inlining failed.

This still does break one declaration rule because we would end up with two
declarations and two symbols for one real symbol.  This cause problems, because
we need to consider this i.e. in testing symbols for equality etc.

We do have a precedent here - the weakrefs are the same evil.  I could generalize
weakref code to also handle warnings(), pattern match this sepcific use (where
we have two symbols for one asm name that differs by warnings), keep the non-warning
global, turn the warning decl static "weakref" translating into the global decl.
Making it static is needed to support different warning messsages across multiple
LTO units - we must not merge the warning annotated symbols.

Now of course this will need a lot of extra special casing of "weakref" because
currently we belive the visibility properties of the duplicated decl that does
not match the visibility properties of the real node.
For weakref I just redirect all refernces to the prevailing declaration if it exists
that solves the problem.  This would disable the warnings.

So while I can implement this, it is not bacportable to 4.8/4.9 and it will likely
trigger interesting side cases for a while, like weakref did.

We can also put warning attribute into gimple call.

Honza
Comment 23 rguenther@suse.de 2014-10-08 07:36:49 UTC
On Tue, 7 Oct 2014, hubicka at ucw dot cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> 
> --- Comment #22 from Jan Hubicka <hubicka at ucw dot cz> ---
> We can also put warning attribute into gimple call.

I like that better.  We can easily add a GF_CALL_WARN / GF_CALL_ERROR
flag, but the question is where to put the warning string...  the
issue with this bugreport is that it might not be on the decl
(because the decl is now replaced with one with/without the attribute).
Maybe if we always "merge" the warning attributes then the call flag
tells us whether to ignore it or not ...

Thus for this particular bug (bogus warning) just adding the flag
as a requirement to emit the warning/error would be enough.  We
may still lose warnings/errors if the wrong decl is picked for
a GF_CALL_WARN call though.

Yeah, now the idea of adding a generic annotation pointer to
gimple stmts will pop up again ... of course I don't like that
very much.

OTOH we had the idea of avoiding warnings from the middle-end
for dead code by queuing warnings to emit on stmts and only
emit them if the stmt is still live at a certain point during
the compilation.

All that said - what about going with the simple GF_CALL_WARN_OR_ERROR
flag to avoid the false "positives"?
Comment 24 Jakub Jelinek 2014-10-08 08:25:34 UTC
But is warning/error attribute the only thing on aliases that can hold extra semantics info (now or in the future)?  I'd say LTO symtab merging should merge what is mergeable, and should leave leave as separate decls with the same asm-name what holds non-mergeable semantics on it.
Say, if you declare some function (or different, just with same asm name) with warning attribute in one TU, with error attribute in another TU and without it on another TU, IMHO those three decls shouldn't be merged together, you should note in cgraph that you have aliases that have the same asm name but different semantics and just ensure that you use the right cgraph nodes and decls in the corresponding callers.
Comment 25 rguenther@suse.de 2014-10-08 10:42:29 UTC
On Wed, 8 Oct 2014, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> 
> --- Comment #24 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> But is warning/error attribute the only thing on aliases that can hold extra
> semantics info (now or in the future)?  I'd say LTO symtab merging should merge
> what is mergeable, and should leave leave as separate decls with the same
> asm-name what holds non-mergeable semantics on it.
> Say, if you declare some function (or different, just with same asm name) with
> warning attribute in one TU, with error attribute in another TU and without it
> on another TU, IMHO those three decls shouldn't be merged together, you should
> note in cgraph that you have aliases that have the same asm name but different
> semantics and just ensure that you use the right cgraph nodes and decls in the
> corresponding callers.

Yes, I tried to fix things in this direction but failed (maybe didn't try 
hard enough).  Basically I'd never merge decls in lto-symtab - tree
merging already merges exactly equivalent function decls - but only
fixup the cgraph for the tree merging that was done.

Richard.
Comment 26 Jan Hubicka 2014-10-08 17:49:27 UTC
> But is warning/error attribute the only thing on aliases that can hold extra
> semantics info (now or in the future)?  I'd say LTO symtab merging should merge
> what is mergeable, and should leave leave as separate decls with the same
> asm-name what holds non-mergeable semantics on it.
> Say, if you declare some function (or different, just with same asm name) with
> warning attribute in one TU, with error attribute in another TU and without it
> on another TU, IMHO those three decls shouldn't be merged together, you should
> note in cgraph that you have aliases that have the same asm name but different
> semantics and just ensure that you use the right cgraph nodes and decls in the
> corresponding callers.

A I tried to explain, it is currently design decision to have  one declaration
and one symtam node for one symbol.  The one decl rule was introduced by
Codesourcery (Zack) in 2003-4. He updated frontends to avoid copying and
dropped code that dealt with duplicated declarations.  Due to lack of sanity
checking some cases remained, like this one (because at that time we did not
really have proper asm name hash).  There are couple open PRs since that time
that was never corrected.

So either we need to fix remaining cases or revisit the decision and audit
backend/middleend for duplicated decls.  There are cases I know that would need
updating
 - symbol equality folding
 - alias analysis
 - Symbol table allowing many to one mapping for symtab entries.
   I think it would be better to avoid duplicated entries in symbol table,
   so we will need way to associate all declarations with a given symbol.
   Probably ont that big deal except for updating code that deals with 
   changing declaration associated with the node and we need to decide what
   declaration control symbol's visibility. 

   Obviously if user provide two declarations, one is static and ohter public,
   we either want to error or do someting sane.
 - we need to produce errors when user defines two different symbols of same name
   (currently we produce invalid asm)
 - anchors
 - Zack dropped some code from dwarf2out
 - Visibility in varasm - those need to follow the main declaration.  I had great
   fun fixing effects of this on AIX

I definitly found the one decl scheme somewhat restrictive, but it also makes
things easier and avoids weird bugs. We could revert this decision, but that is
a project.

Concerning Richards plan to annotate statements with warning strings, I think
we could follow same scheme as EH regions and histograms does - i.e. have on
side hash table annotating statements.

For IPA I am trying to convince Martin Liska to introduce symtab annotation template
for me that makes it easy to add data to a symbol that is removed/duplicated along with
the symbol.  Cgraph has the hook API for it, but convenient C++ wrap would be great.

Here I think we want something similar and convert the existing EH/histograms to it.

Honza
Comment 27 Jakub Jelinek 2014-12-19 13:33:49 UTC
GCC 4.8.4 has been released.
Comment 28 Richard Biener 2015-01-19 13:16:00 UTC
Still broken :(
Comment 29 Richard Biener 2015-02-11 08:29:36 UTC
*** Bug 62249 has been marked as a duplicate of this bug. ***
Comment 30 Richard Biener 2015-02-11 08:33:42 UTC
So, do we really want to go without this fixed again, for GCC 5?  Honza?  After
all this is an underlying wrong-code issue!  (wrong function is picked as prevailing)
Comment 31 Jan Hubicka 2015-02-11 09:15:05 UTC
> So, do we really want to go without this fixed again, for GCC 5?  Honza?  After
> all this is an underlying wrong-code issue!  (wrong function is picked as
> prevailing)

Well, I have only two hands and do not see reasonably simple solution. Will look into it
again.  How this cause wrong code?

Honza
Comment 32 rguenther@suse.de 2015-02-11 10:05:49 UTC
On Wed, 11 Feb 2015, hubicka at ucw dot cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> 
> --- Comment #31 from Jan Hubicka <hubicka at ucw dot cz> ---
> > So, do we really want to go without this fixed again, for GCC 5?  Honza?  After
> > all this is an underlying wrong-code issue!  (wrong function is picked as
> > prevailing)
> 
> Well, I have only two hands and do not see reasonably simple solution. Will
> look into it
> again.  How this cause wrong code?

Hmm, maybe it can't (the "aliases" map to the same symbol).  But at least
if I produce another decl with say, attribute(regparm), and that gets
picked even though I didn't call it then it would be wrong-code
(of course that decl is technically invalid as the symbol it refers to
has different calling conventions).
Comment 33 rguenther@suse.de 2015-02-11 10:08:20 UTC
On Wed, 11 Feb 2015, Richard Biener wrote:

> On Wed, 11 Feb 2015, hubicka at ucw dot cz wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
> > 
> > --- Comment #31 from Jan Hubicka <hubicka at ucw dot cz> ---
> > > So, do we really want to go without this fixed again, for GCC 5?  Honza?  After
> > > all this is an underlying wrong-code issue!  (wrong function is picked as
> > > prevailing)
> > 
> > Well, I have only two hands and do not see reasonably simple solution. Will
> > look into it
> > again.  How this cause wrong code?
> 
> Hmm, maybe it can't (the "aliases" map to the same symbol).  But at least
> if I produce another decl with say, attribute(regparm), and that gets
> picked even though I didn't call it then it would be wrong-code
> (of course that decl is technically invalid as the symbol it refers to
> has different calling conventions).

Like

extern void fooreg (int i) __attribute__((regparm)) __asm__ ("bar");
extern void foo (int i) __asm__ ("bar");

void forward (int i, int reg)
{
  if (reg)
    fooreg (i);
  else
    foo (i);
}

and use it like LD_PRELOAD=libwithregparm.so ./test --regparm.

Poor-mans ifuncs ;)
Comment 34 Zack Weinberg 2015-02-11 16:30:00 UTC
> As I tried to explain, it is currently design decision to have one declaration
> and one symtam node for one symbol.  The one decl rule was introduced by
> Codesourcery (Zack) in 2003-4. He updated frontends to avoid copying and
> dropped code that dealt with duplicated declarations.  Due to lack of sanity
> checking some cases remained, like this one (because at that time we did not
> really have proper asm name hash).  There are couple open PRs since that time
> that was never corrected.

It's been long enough that I don't recall precisely what my goals were, but I don't think it was my intention to enforce a "one decl per asm name" rule at the time.  I was trying to deal with C front-end issues: according to https://gcc.gnu.org/ml/gcc-patches/2004-01/msg00808.html, "at least bug 12267, bug 12336, bug 12373, bug 12391, bug 12560, bug 12934, bug 13129".  My recollection is that the fundamental problem was getting more than one TREE_DECL object per *declared name* in -funit-at-a-time mode, which was shiny and new back then!

... Digging through the mailing list archives, maybe I'm remembering wrong.  https://gcc.gnu.org/ml/gcc-patches/2004-03/msg01440.html has

> The old code took the copying
> approach; this was bad because it violated the basic assumption made
> elsewhere in the compiler that there is exactly one DECL node for each
> assembly-level symbol.  Hence all the bugs.

I do not remember anymore what parts of the compiler were making those assumptions, but I'm guessing it was probably the debug information generators, or mostly those.  Anyhow, I don't think y'all should be taking a decision I made for the C front end under time and peer pressure (people were *really mad* at me for leaving some of those bugs unfixed for several releases in a row) in 2004 as a permanent compiler-wide design constraint ten years later :)

(Note also bug 13801, and the discussion of its fix, here: https://gcc.gnu.org/ml/gcc-patches/2004-08/msg00085.html -- we wound up having to back down from the "not only is there only one DECL per assembly level symbol, it only ever accumulates information" rule that I originally wanted, due to C90 conformance problems.)
Comment 35 Jan Hubicka 2015-03-03 19:12:46 UTC
Zack,
happy to hear from you again! Indeed the problem back was quite sloppy and we kind of mixed up symbols, assembler names and declarations in not well defined way.

I think the safest way to go is to build on the alias machinery.  For weakref we already have sense of "syntactic alias" (those that ends up translated to their target symbols) and I did some auditing recently (motivated by ICF bugs). Currently we have  node->weakref saying if symbol is weakref and we do have good part of code aware of this.  I guess we can have node->syntactic_alias (better name welcome, perhaps transparent?) that express the fact that alias should get translated to the final symbol during RTL output the same way as we do weakref on targets where they are not supported.
Then it is a question where we want to translate the duplicated declarations into these aliases.  I guess I can do it within the visibility itself or the FEs can be responsible for it.

We can also get more fancy and try to solve the GNU extern inline issues - have a syntactic alias with also has boddy associated with it.

I will try to start pushing things this direction.
Comment 36 Jan Hubicka 2015-03-20 20:40:14 UTC
This is probably too risky to fix for 5.1 (https://gcc.gnu.org/ml/gcc/2015-03/msg00241.html) though I am having WIP patch now - we need to introduce transparent alias support and make lto-symtab to turn incompatible decls into transparent aliases.

I hope to have patch ready in a week for consideration. Chromium is using fortify, so it would be nice to fix this.
Comment 37 Richard Biener 2015-06-23 08:16:22 UTC
The gcc-4_8-branch is being closed, re-targeting regressions to 4.9.3.
Comment 38 Jakub Jelinek 2015-06-26 19:57:55 UTC
GCC 4.9.3 has been released.
Comment 39 Jan Hubicka 2015-12-07 17:37:26 UTC
Author: hubicka
Date: Mon Dec  7 17:36:54 2015
New Revision: 231373

URL: https://gcc.gnu.org/viewcvs?rev=231373&root=gcc&view=rev
Log:
	PR ipa/61886
	* symtab.c (ultimate_transparent_alias_target): New inline function.
	(symbol_table::assembler_names_equal_p): New method; break out from ...
	(symbol_table::decl_assembler_name_equal): ... here.
	(symbol_table::change_decl_assembler_name): Also update names and
	translation links of transparent aliases.
	(symtab_node::dump_base): Dump transparent_alias.
	(symtab_node::verify_base): Implement basic transparent alias
	verification.
	(symtab_node::make_decl_local): Support localization of weakrefs;
	recurse to transparent aliases; set TREE_STATIC.
	(symtab_node::ultimate_alias_target_1): Handle visibility of
	transparent aliases.
	(symtab_node::resolve_alias): New parmaeter transparent; handle
	transparent aliases; recurse to aliases of aliases to fix comdat
	groups.
	(symtab_node::get_partitioning_class): Handle transparent aliases.
	* ipa-visibility.c (cgraph_externally_visible_p,
	varpool_node::externally_visible_p): Visibility of transparent alias
	depends on its target.
	(function_and_variable_visibility): Do not tweak visibility of
	transparent laiases.
	(function_and_variable_visibility): Likewise.
	* ipa.c (symbol_table::remove_unreachable_nodes): Clear
	transparent_alias flag.
	* alias.c (cgraph_node::create_alias, cgraph_node::get_availability):
	Support transparent aliases.
	* cgraph.h (symtab_node): Update prototype of resolve_alias;
	add transparent_alias flag.
	(symbol_table: Add assembler_names_equal_p.
	(symtab_node::real_symbol_p): Skip transparent aliases.
	* cgraphunit.c (cgraph_node::reset): Reset transparent_alias flag.
	(handle_alias_pairs): Set transparent_alias for weakref.
	(cgraph_node::assemble_thunks_and_aliases): Do not asemble transparent
	aliases.
	* lto-cgraph.c (lto_output_node): When outputting same_comdat_group
	skip symbols not put into boundary; stream transparent_alias.
	(lto_output_varpool_node): Likewise.
	(input_overwrite_node, input_varpool_node): Stream transparent alias.
	* varpool.c (ctor_for_folding, varpool_node::get_availability,
	varpool_node::assemble_aliases,
	symbol_table::remove_unreferenced_decls): Handle transparent aliase.
	(varpool_node::create_alias): Set transparent_alias.

	* lto-partition.c (add_symbol_to_partition_1, contained_in_symbol,
	rename_statics, rename_statics): Handle transparent aliases.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraph.c
    trunk/gcc/cgraph.h
    trunk/gcc/cgraphunit.c
    trunk/gcc/ipa-visibility.c
    trunk/gcc/ipa.c
    trunk/gcc/lto-cgraph.c
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lto-partition.c
    trunk/gcc/symtab.c
    trunk/gcc/tree.c
    trunk/gcc/varpool.c
Comment 40 Jan Hubicka 2015-12-08 20:47:14 UTC
Author: hubicka
Date: Tue Dec  8 20:46:42 2015
New Revision: 231425

URL: https://gcc.gnu.org/viewcvs?rev=231425&root=gcc&view=rev
Log:
	PR ipa/61886
	* lto-partition.c (add_symbol_to_partition_1): Transparent aliases
	are not part of the definition.
	(contained_in_symbol): Likewise.
	(promote_symbol): When promoting a symbol also promote all transparent
	aliases.
	(rename_statics): Weakref needs unique name, too.

Modified:
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lto-partition.c
Comment 41 Jan Hubicka 2015-12-08 22:01:27 UTC
Author: hubicka
Date: Tue Dec  8 22:00:55 2015
New Revision: 231427

URL: https://gcc.gnu.org/viewcvs?rev=231427&root=gcc&view=rev
Log:
	PR ipa/61886
	* symtab.c (symtab_node::verify_base): Fix thinko in a conditional.
	(symtab_node::noninterposable_alias): Do not accept transparent
	aliases.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/symtab.c
Comment 42 Jan Hubicka 2015-12-08 22:02:13 UTC
Author: hubicka
Date: Tue Dec  8 22:01:41 2015
New Revision: 231428

URL: https://gcc.gnu.org/viewcvs?rev=231428&root=gcc&view=rev
Log:

	PR ipa/61886
	* ipa-visibility.c (can_replace_by_local_alias): Look through transparent
	aliaes; refuse weakrefs.
	(update_visibility_by_resolution_info): Skip transparent aliases in the
	analysis part

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-visibility.c
Comment 43 Jan Hubicka 2015-12-08 22:02:55 UTC
Author: hubicka
Date: Tue Dec  8 22:02:23 2015
New Revision: 231429

URL: https://gcc.gnu.org/viewcvs?rev=231429&root=gcc&view=rev
Log:

	PR ipa/61886
	* varpool.c (varpool_node::get_availability): Recurse only on
	weakrefs with definition in the target.
	(symbol_table::remove_unreferenced_decls): Keep aliases in the boundary.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/varpool.c
Comment 44 Jan Hubicka 2015-12-08 22:04:07 UTC
Author: hubicka
Date: Tue Dec  8 22:03:36 2015
New Revision: 231430

URL: https://gcc.gnu.org/viewcvs?rev=231430&root=gcc&view=rev
Log:
	PR ipa/61886
	* lto-cgraph.c (compute_ltrans_boundary): Add transparent alias targets
	into the boundary.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lto-cgraph.c
Comment 45 Jan Hubicka 2015-12-09 02:15:38 UTC
Author: hubicka
Date: Wed Dec  9 02:15:05 2015
New Revision: 231438

URL: https://gcc.gnu.org/viewcvs?rev=231438&root=gcc&view=rev
Log:


	PR ipa/61886
	* lto-streamer.h (lto_symtab_merge_decls, lto_symtab_merge_symbols,
	lto_symtab_prevailing_decl): MOve to lto-symtab.h.
	* lto-streamer-out.c (DFS::DFS_write_tree_body): Check that
	DECL_ABSTRACT_ORIGIN is not error_mark_node.

	* lto-symtab.c: Include lto-symtab.h.
	(lto_cgraph_replace_node): Do not merge profiles here.
	(lto_symtab_merge_p): New function.
	(lto_symtab_merge_decls_2): Honor lto_symtab_merge_p.
	(lto_symtab_merge_symbols_1): Turn unmerged decls into transparent
	aliases.
	(lto_symtab_merge_symbols): Do not clear node->aux; we no longer use it.
	(lto_symtab_prevailing_decl): Move to lto-symtab.h; rewrite.
	* lto.c: Include lto-symtab.h
	* lto-symtab.h: New.

Added:
    trunk/gcc/lto/lto-symtab.h
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lto-streamer-out.c
    trunk/gcc/lto-streamer.h
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lto-symtab.c
    trunk/gcc/lto/lto.c
Comment 46 Jan Hubicka 2015-12-09 05:07:51 UTC
Author: hubicka
Date: Wed Dec  9 05:07:18 2015
New Revision: 231440

URL: https://gcc.gnu.org/viewcvs?rev=231440&root=gcc&view=rev
Log:

	PR ipa/61886
	* symtab.c (symtab_node::equal_address_to): New parameter
	MEMORY_ACCESSED.
	* cgraph.h (symtab_node::equal_address_to): Update prototype.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraph.h
    trunk/gcc/symtab.c
Comment 47 Jan Hubicka 2015-12-09 07:34:48 UTC
Author: hubicka
Date: Wed Dec  9 07:34:16 2015
New Revision: 231442

URL: https://gcc.gnu.org/viewcvs?rev=231442&root=gcc&view=rev
Log:

	PR ipa/61886
	PR middle-end/25140
	* ipa-reference.c (is_improper): Break out from ...
	(is_proper_for_analysis): ... here; fix WRT aliases.
	(analyze_function, generate_summary,
	ipa_reference_write_optimization_summary,
	ipa_reference_read_optimization_summary): Use ipa_reference_var_uid.
	* ipa-refrence.h (ipa_reference_var_uid): New inline.
	* tree-ssa-alias.c (ref_maybe_used_by_call_p_1,
	call_may_clobber_ref_p_1): Use ipa_reference_var_uid.

	* gcc.c-torture/execute/alias-3.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/alias-3.c
Modified:
    trunk/gcc/ipa-reference.c
    trunk/gcc/ipa-reference.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-alias.c
Comment 48 Jan Hubicka 2015-12-09 19:30:11 UTC
Author: hubicka
Date: Wed Dec  9 19:29:38 2015
New Revision: 231471

URL: https://gcc.gnu.org/viewcvs?rev=231471&root=gcc&view=rev
Log:

	PR ipa/61886
	PR middle-end/25140
	* ipa-reference.c (ipa_reference_get_not_read_global,
	ipa_reference_get_not_read_global): Fix WRT aliases.
	(is_improper): Break out from ...
	(is_proper_for_analysis): ... here; fix WRT aliases.
	(analyze_function, generate_summary,
	ipa_reference_write_optimization_summary,
	ipa_reference_read_optimization_summary): Use ipa_reference_var_uid.
	* ipa-refrence.h (ipa_reference_var_uid): New inline.
	* tree-ssa-alias.c: Revert my accidental previous commit.
	(ref_maybe_used_by_call_p_1,
	call_may_clobber_ref_p_1): Use ipa_reference_var_uid.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-reference.c
    trunk/gcc/tree-ssa-alias.c
Comment 49 Jan Hubicka 2015-12-09 19:40:41 UTC
Author: hubicka
Date: Wed Dec  9 19:40:10 2015
New Revision: 231474

URL: https://gcc.gnu.org/viewcvs?rev=231474&root=gcc&view=rev
Log:

	PR ipa/61886
	* ipa-visibility.c (function_and_variable_visibility): Fix vtable
	rewritting guard.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-visibility.c
Comment 50 Jan Hubicka 2015-12-09 23:28:33 UTC
Author: hubicka
Date: Wed Dec  9 23:28:01 2015
New Revision: 231478

URL: https://gcc.gnu.org/viewcvs?rev=231478&root=gcc&view=rev
Log:

	PR ipa/61886
	PR middle-end/25140
	* tree-ssa-alias.c (ptr_deref_may_alias_decl_p): Use compare_base_decls
	(nonoverlapping_component_refs_of_decl_p): Update sanity check.
	(decl_refs_may_alias_p): Use compare_base_decls.
	* alias.c: Include cgraph.h
	(rtx_equal_for_memref_p): Use rtx_equal_for_memref_p.
	(compare_base_decls): New function.
	(base_alias_check): Likewise.
	(memrefs_conflict_p): Likewise.
	(nonoverlapping_memrefs_p): Likewise.
	* alias.h (compare_base_decls): Declare.

	* gcc.c-torture/execute/alias-2.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/alias-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/alias.c
    trunk/gcc/alias.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-alias.c
Comment 51 Jan Hubicka 2015-12-10 23:57:47 UTC
Author: hubicka
Date: Thu Dec 10 23:57:15 2015
New Revision: 231548

URL: https://gcc.gnu.org/viewcvs?rev=231548&root=gcc&view=rev
Log:
	PR ipa/61886
	* lto-symtab.c (lto_symtab_merge_p): Avoid merging across different
	values of error and warning attributes.
	* gcc.dg/lto/pr61886_0.c: New testcase

Added:
    trunk/gcc/testsuite/gcc.dg/lto/pr61886_0.c
Modified:
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lto-symtab.c
    trunk/gcc/testsuite/ChangeLog
Comment 52 Jan Hubicka 2015-12-10 23:59:44 UTC
We no longer merge the decls, so hopefully this is fixed, but it would be nice to give it a try on the original packages that failed.  We still may lose/provoke some warnings during IPA optimizations I suppose as IPA code does not assume this kind of side effect for aliases, but that is not an LTO specific issue anymore.
Comment 53 mwahab 2015-12-16 13:14:10 UTC
(In reply to Jan Hubicka from comment #52)
> We no longer merge the decls, so hopefully this is fixed, but it would be
> nice to give it a try on the original packages that failed.  We still may
> lose/provoke some warnings during IPA optimizations I suppose as IPA code
> does not assume this kind of side effect for aliases, but that is not an LTO
> specific issue anymore.

The new test case alias-2.c fails for aarch64.

Compiling from the command line with gcc -O2 -fdump-tree-all, I get the following for the tree.optimized pass
-----
;; Function main (main, funcdef_no=0, decl_uid=2753, cgraph_uid=0, symbol_order=3) (executed once)

main ()
{
  int off.0_2;

  <bb 2>:
  off.0_2 = off;
  b[off.0_2] = 1;
  a[off.0_2] = 2;
  __builtin_abort ();

}
----

Matthew
Comment 54 ktkachov 2015-12-16 13:35:41 UTC
The new test failure is PR 68832
Comment 55 Thomas Preud'homme 2015-12-29 06:12:48 UTC
Shouldn't gcc.dg/lto/pr61886_0.c be run only for libc having __fread_chk? It fails on arm-none-eabi targets because newlib doesn't have __fread_chk:

<artificial>:(.text.startup+0x18): undefined reference to `__fread_chk'
FAIL: gcc.dg/lto/pr61886 c_lto_pr61886_0.o-c_lto_pr61886_0.o link,  -flto -O2 -Werror
Comment 56 Thomas Preud'homme 2015-12-29 06:26:24 UTC
My apologize, this is actually already reported as PR68913
Comment 57 Jan Hubicka 2016-01-18 17:46:13 UTC
The alias-2.c should be now fixed on targets with anchors.
Comment 58 Jan Hubicka 2016-01-19 12:48:37 UTC
Fixed.
Comment 59 Richard Biener 2016-01-19 12:58:10 UTC
For GCC 6 only though.