Re: [PATCH] error on missing LTO symbols

[Tom de Vries]

On 17-12-18 13:46, Martin Jambor wrote:
> Hi,
> 
> On Fri, Dec 14 2018, Jakub Jelinek wrote:
>> On Fri, Dec 14, 2018 at 10:21:35AM +0100, Tom de Vries wrote:
>>> --- /dev/null
>>> +++ b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
>>> @@ -0,0 +1,21 @@
>>> +/* { dg-do link } */
>>> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
>>> +
>>> +int results[2000]; /* { dg-error "variable 'results' has been referenced 
>>> in offloaded code but hasn't been marked to be included in the offloaded 
>>> code" } */
>>> +
>>> +#pragma omp declare target
>>> +void  __attribute__((noinline, noclone))
>>> +baz (int i)
>>> +{
>>> +  results[i]++;
>>> +}
>>> +#pragma omp end declare target
>>> +
>>> +int
>>> +main ()
>>> +{
>>> +#pragma omp target
>>> +#pragma omp for
>>> +  for (int i = 0; i < 2000; i++)
>>> +baz (i);
>>> +}
>>
>> Ah, one more thing, the testcase doesn't really fail when offloading isn't
>> configured, so it would need some effective target or something that
>> it actually does the offloading.  And not really sure about shared memory
>> offloading like hsail, that doesn't really need the variables declared
>> either, just the functions.
>>
> 
> so IIUC and IIRC, the testcase should have:
> 
> { dg-require-effective-target offload_device_nonshared_as }
> 

Hi Martin,

thanks for the confirmation. That's what's been used in the committed
version.

- Tom

Re: [PATCH] error on missing LTO symbols

[Martin Jambor]

Hi,

On Fri, Dec 14 2018, Jakub Jelinek wrote:
> On Fri, Dec 14, 2018 at 10:21:35AM +0100, Tom de Vries wrote:
>> --- /dev/null
>> +++ b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
>> @@ -0,0 +1,21 @@
>> +/* { dg-do link } */
>> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
>> +
>> +int results[2000]; /* { dg-error "variable 'results' has been referenced in 
>> offloaded code but hasn't been marked to be included in the offloaded code" 
>> } */
>> +
>> +#pragma omp declare target
>> +void  __attribute__((noinline, noclone))
>> +baz (int i)
>> +{
>> +  results[i]++;
>> +}
>> +#pragma omp end declare target
>> +
>> +int
>> +main ()
>> +{
>> +#pragma omp target
>> +#pragma omp for
>> +  for (int i = 0; i < 2000; i++)
>> +baz (i);
>> +}
>
> Ah, one more thing, the testcase doesn't really fail when offloading isn't
> configured, so it would need some effective target or something that
> it actually does the offloading.  And not really sure about shared memory
> offloading like hsail, that doesn't really need the variables declared
> either, just the functions.
>

so IIUC and IIRC, the testcase should have:

{ dg-require-effective-target offload_device_nonshared_as }

Thanks,

Martin

Re: [PATCH] error on missing LTO symbols

[Jakub Jelinek]

On Fri, Dec 14, 2018 at 02:17:49PM +0100, Tom de Vries wrote:
> Build x86_64 and reg-tested libgomp.
> 
> 2018-12-13  Tom de Vries  
> 
>   * lto-cgraph.c (verify_node_partition): New function.
>   (input_overwrite_node, input_varpool_node): Use verify_node_partition.
> 
>   * testsuite/libgomp.c-c++-common/function-not-offloaded-aux.c: New test.
>   * testsuite/libgomp.c-c++-common/function-not-offloaded.c: New test.
>   * testsuite/libgomp.c-c++-common/variable-not-offloaded.c: New test.
>   * testsuite/libgomp.oacc-c-c++-common/function-not-offloaded.c: New 
> test.
>   * testsuite/libgomp.oacc-c-c++-common/variable-not-offloaded.c: New 
> test.

> +/* Verify the partitioning of a varpool_node or cgraph_node with DECL and 
> NAME,
> +   as specified by IN_OTHER_PARTITION and USED_FROM_OTHER_PARTITION.  */
> +
> +static inline void
> +verify_node_partition (symtab_node *node)

Please update the function comment.

Ok with that change.

Jakub

Re: [PATCH] error on missing LTO symbols

[Tom de Vries]

On 14-12-18 14:08, Jakub Jelinek wrote:
> On Fri, Dec 14, 2018 at 02:07:18PM +0100, Tom de Vries wrote:
>> Done, using offload_device_nonshared_as for
>> libgomp.c-c++-common/variable-not-offloaded.c and
>> openacc_nvidia_accel_configured for
>> libgomp.oacc-c-c++-common/function-not-offloaded.c.
>>
>>> Otherwise LGTM.
>>
>> Updated patch OK?
> 
> ENOPATCH

Sorry, here it is.

Thanks,
- Tom
[offloading] Error on missing symbols

When compiling an OpenMP or OpenACC program containing a reference in the
offloaded code to a symbol that has not been included in the offloaded code,
the offloading compiler may ICE in lto1.

Fix this by erroring out instead, mentioning the problematic symbol:
...
error: variable 'var' has been referenced in offloaded code but hasn't
  been marked to be included in the offloaded code
lto1: fatal error: errors during merging of translation units
compilation terminated.
...

Build x86_64 with nvptx accelerator and reg-tested libgomp.

Build x86_64 and reg-tested libgomp.

2018-12-13  Tom de Vries  

	* lto-cgraph.c (verify_node_partition): New function.
	(input_overwrite_node, input_varpool_node): Use verify_node_partition.

	* testsuite/libgomp.c-c++-common/function-not-offloaded-aux.c: New test.
	* testsuite/libgomp.c-c++-common/function-not-offloaded.c: New test.
	* testsuite/libgomp.c-c++-common/variable-not-offloaded.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/function-not-offloaded.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/variable-not-offloaded.c: New test.

---
 gcc/lto-cgraph.c   | 40 ++
 .../function-not-offloaded-aux.c   | 12 +++
 .../libgomp.c-c++-common/function-not-offloaded.c  | 16 +
 .../libgomp.c-c++-common/variable-not-offloaded.c  | 19 ++
 .../function-not-offloaded.c   | 18 ++
 .../variable-not-offloaded.c   | 17 +
 6 files changed, 115 insertions(+), 7 deletions(-)

diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index 8cc3c75..546abaeff48 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1091,6 +1091,37 @@ output_offload_tables (void)
 }
 }
 
+/* Verify the partitioning of a varpool_node or cgraph_node with DECL and NAME,
+   as specified by IN_OTHER_PARTITION and USED_FROM_OTHER_PARTITION.  */
+
+static inline void
+verify_node_partition (symtab_node *node)
+{
+  if (flag_ltrans)
+return;
+
+#ifdef ACCEL_COMPILER
+  if (node->in_other_partition)
+{
+  if (TREE_CODE (node->decl) == FUNCTION_DECL)
+	error_at (DECL_SOURCE_LOCATION (node->decl),
+		  "function %qs has been referenced in offloaded code but"
+		  " hasn%'t been marked to be included in the offloaded code",
+		  node->name ());
+  else if (VAR_P (node->decl))
+	error_at (DECL_SOURCE_LOCATION (node->decl),
+		  "variable %qs has been referenced in offloaded code but"
+		  " hasn%'t been marked to be included in the offloaded code",
+		  node->name ());
+  else
+	gcc_unreachable ();
+}
+#else
+  gcc_assert (!node->in_other_partition
+	  && !node->used_from_other_partition);
+#endif
+}
+
 /* Overwrite the information in NODE based on FILE_DATA, TAG, FLAGS,
STACK_SIZE, SELF_TIME and SELF_SIZE.  This is called either to initialize
NODE or to replace the values in it, for instance because the first
@@ -1153,9 +1184,7 @@ input_overwrite_node (struct lto_file_decl_data *file_data,
   node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution,
  LDPR_NUM_KNOWN);
   node->split_part = bp_unpack_value (bp, 1);
-  gcc_assert (flag_ltrans
-	  || (!node->in_other_partition
-		  && !node->used_from_other_partition));
+  verify_node_partition (node);
 }
 
 /* Return string alias is alias of.  */
@@ -1366,10 +1395,7 @@ input_varpool_node (struct lto_file_decl_data *file_data,
 node->set_section_for_node (section);
   node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution,
 	LDPR_NUM_KNOWN);
-  gcc_assert (flag_ltrans
-	  || (!node->in_other_partition
-		  && !node->used_from_other_partition));
-
+  verify_node_partition (node);
   return node;
 }
 
diff --git a/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded-aux.c b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded-aux.c
new file mode 100644
index 000..b8aa3da48a1
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded-aux.c
@@ -0,0 +1,12 @@
+/* { dg-skip-if "" { *-*-* } } */
+
+#pragma omp declare target
+extern int var;
+#pragma omp end declare target
+
+void __attribute__((noinline, noclone))
+foo (void)
+{
+  var++;
+}
+
diff --git a/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded.c b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded.c
new file mode 100644
index 000..9e59ef8864e
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded.c
@@ -0,0 +1,16 @@
+/* { dg-do link } */
+/* { 

Re: [PATCH] error on missing LTO symbols

[Jakub Jelinek]

On Fri, Dec 14, 2018 at 02:07:18PM +0100, Tom de Vries wrote:
> Done, using offload_device_nonshared_as for
> libgomp.c-c++-common/variable-not-offloaded.c and
> openacc_nvidia_accel_configured for
> libgomp.oacc-c-c++-common/function-not-offloaded.c.
> 
> > Otherwise LGTM.
> 
> Updated patch OK?

ENOPATCH

Jakub

Re: [PATCH] error on missing LTO symbols

[Tom de Vries]

[ cc-ing HSAIL maintainer ]

On 14-12-18 10:54, Jakub Jelinek wrote:
> On Fri, Dec 14, 2018 at 10:21:35AM +0100, Tom de Vries wrote:
>> Build and reg-tested on x86_64 with nvptx accelerator.
>>
>> 2018-12-13  Tom de Vries  
>>
>>  * lto-cgraph.c (verify_node_partition): New function.
>>  (input_overwrite_node, input_varpool_node): Use verify_node_partition.
>>
>>  * testsuite/libgomp.c-c++-common/function-not-offloaded.c: New test.
>>  * testsuite/libgomp.c-c++-common/variable-not-offloaded.c: New test.
> 
>> +  if (TREE_CODE (decl) == FUNCTION_DECL
>> +  || TREE_CODE (decl) == VAR_DECL)
>> +error_at (DECL_SOURCE_LOCATION (decl),
>> +  "%s %qs has been referenced in offloaded code but"
>> +  " hasn't been marked to be included in the offloaded code",
>> +  TREE_CODE (decl) == FUNCTION_DECL ? "function" : "variable",
>> +  name);
> 
> This is translation unfriendly.  Please just do:
>   if (TREE_CODE (decl) == FUNCTION_DECL)
>   error_at (...);
>   else if (VAR_P (decl))
>   error_at (...);
>   else
>   gcc_unreachable ();
> (also note VAR_P).  And, please use hasn%'t instead of hasn't.
> 

Done.

>> @@ -1153,9 +1184,8 @@ input_overwrite_node (struct lto_file_decl_data 
>> *file_data,
>>node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution,
>>   LDPR_NUM_KNOWN);
>>node->split_part = bp_unpack_value (bp, 1);
>> -  gcc_assert (flag_ltrans
>> -  || (!node->in_other_partition
>> -  && !node->used_from_other_partition));
>> +  verify_node_partition (node->decl, node->name (), 
>> node->in_other_partition,
>> + node->used_from_other_partition);
>>  }
> 
> Why are you passing all these arguments to that function (especially calling
> node->name () even when you don't know if it will be needed or not)?
> Doesn't both cgraph_node and varpool_node inherit from symtab_node, which
> has all these 3 fields as well as name () method?
> So, I think if verify_node_partition takes a symtab_node *node argument
> and the callers just both do
>   verify_node_partition (node);
> it should work fine.  I'd make it inline too.
> 

Done.

>> --- /dev/null
>> +++ b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded.c
>> @@ -0,0 +1,21 @@
>> +/* { dg-do link } */
>> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
>> +
>> +#pragma omp declare target
>> +int results[2000];
>> +#pragma omp end declare target
>> +
>> +void __attribute__((noinline, noclone))
>> +baz (int i) /* { dg-error "function 'baz' has been referenced in offloaded 
>> code but hasn't been marked to be included in the offloaded code" } */
>> +{
>> +  results[i]++;
>> +}
>> +
>> +int
>> +main ()
>> +{
>> +#pragma omp target
>> +#pragma omp for
>> +  for (int i = 0; i < 2000; i++)
>> +baz (i);
>> +}
> 
> Note, this will be well defined in OpenMP 5.0, just the support isn't there.
> The spec says:
> "If a function (C, C++, Fortran) or subroutine (Fortran) is referenced in a 
> target construct then
> that function or subroutine is treated as if its name had appeared in a to 
> clause on a
> declare target directive."
> and
> "If a function is referenced in a function that appears as a list item in a 
> to clause on a
> declare target directive then the name of the referenced function is treated 
> as if it had
> appeared in a to clause on a declare target directive.
> 
> If a variable with static storage duration or a function (except lambda for 
> C++) is referenced in the
> initializer expression list of a variable with static storage duration that 
> appears as a list item in a to
> clause on a declare target directive then the name of the referenced variable 
> or function is
> treated as if it had appeared in a to clause on a declare target directive."
> 
> so for functions it should work through implicit propagation of the
> "omp declare target" attribute.
> 
> Can't find a restriction I'd expect to see that if a declaration of a
> function or variable is marked declare target then the definition has to be
> as well, will talk to omp-lang.
> 
> In any case, for the above testcase it might be better to split it into two
> sources with dg-auxiliary-source, declare baz in the source with main and
> define in another file (where declare instead of define the variable).
>

Done, but that ends up not triggering the introduced error condition
anymore. Instead we generate an "unresolved symbol foo".

So I've added the corresponding OpenACC test-cases as well (and the
introduced error triggers only for OpenACC functions and OpenMP variables).

>> diff --git a/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c 
>> b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
>> new file mode 100644
>> index 000..c2e1d57adea
>> --- /dev/null
>> +++ b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
>> @@ 

Re: [PATCH] error on missing LTO symbols

[Thomas Schwinge]

Hi Tom!

Thanks for looking into this one!

Just one quick comment:

On Fri, 14 Dec 2018 10:21:35 +0100, Tom de Vries  wrote:
> --- a/gcc/lto-cgraph.c
> +++ b/gcc/lto-cgraph.c

> +#ifdef ACCEL_COMPILER
> +  if (in_other_partition)
> +{
> +  if (TREE_CODE (decl) == FUNCTION_DECL
> +   || TREE_CODE (decl) == VAR_DECL)
> + error_at (DECL_SOURCE_LOCATION (decl),
> +   "%s %qs has been referenced in offloaded code but"
> +   " hasn't been marked to be included in the offloaded code",
> +   TREE_CODE (decl) == FUNCTION_DECL ? "function" : "variable",
> +   name);
> +  else
> + gcc_unreachable ();
> +}
> +#else
> +  gcc_assert (!in_other_partition
> +   && !used_from_other_partition);
> +#endif

Given the above...

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded.c
> @@ -0,0 +1,21 @@
> +/* { dg-do link } */
> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
> +
> +#pragma omp declare target
> +int results[2000];
> +#pragma omp end declare target
> +
> +void __attribute__((noinline, noclone))
> +baz (int i) /* { dg-error "function 'baz' has been referenced in offloaded 
> code but hasn't been marked to be included in the offloaded code" } */

I think this error will trigger only if offloading compilation is
enabled, so this error or the whole test case needs to be conditionalized
on that?

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
> @@ -0,0 +1,21 @@
> +/* { dg-do link } */
> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
> +
> +int results[2000]; /* { dg-error "variable 'results' has been referenced in 
> offloaded code but hasn't been marked to be included in the offloaded code" } 
> */

Likewise.


Grüße
 Thomas

Re: [PATCH] error on missing LTO symbols

[Jakub Jelinek]

On Fri, Dec 14, 2018 at 10:21:35AM +0100, Tom de Vries wrote:
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
> @@ -0,0 +1,21 @@
> +/* { dg-do link } */
> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
> +
> +int results[2000]; /* { dg-error "variable 'results' has been referenced in 
> offloaded code but hasn't been marked to be included in the offloaded code" } 
> */
> +
> +#pragma omp declare target
> +void  __attribute__((noinline, noclone))
> +baz (int i)
> +{
> +  results[i]++;
> +}
> +#pragma omp end declare target
> +
> +int
> +main ()
> +{
> +#pragma omp target
> +#pragma omp for
> +  for (int i = 0; i < 2000; i++)
> +baz (i);
> +}

Ah, one more thing, the testcase doesn't really fail when offloading isn't
configured, so it would need some effective target or something that
it actually does the offloading.  And not really sure about shared memory
offloading like hsail, that doesn't really need the variables declared
either, just the functions.

Jakub

Re: [PATCH] error on missing LTO symbols

[Jakub Jelinek]

On Fri, Dec 14, 2018 at 10:21:35AM +0100, Tom de Vries wrote:
> Build and reg-tested on x86_64 with nvptx accelerator.
> 
> 2018-12-13  Tom de Vries  
> 
>   * lto-cgraph.c (verify_node_partition): New function.
>   (input_overwrite_node, input_varpool_node): Use verify_node_partition.
> 
>   * testsuite/libgomp.c-c++-common/function-not-offloaded.c: New test.
>   * testsuite/libgomp.c-c++-common/variable-not-offloaded.c: New test.

> +  if (TREE_CODE (decl) == FUNCTION_DECL
> +   || TREE_CODE (decl) == VAR_DECL)
> + error_at (DECL_SOURCE_LOCATION (decl),
> +   "%s %qs has been referenced in offloaded code but"
> +   " hasn't been marked to be included in the offloaded code",
> +   TREE_CODE (decl) == FUNCTION_DECL ? "function" : "variable",
> +   name);

This is translation unfriendly.  Please just do:
  if (TREE_CODE (decl) == FUNCTION_DECL)
error_at (...);
  else if (VAR_P (decl))
error_at (...);
  else
gcc_unreachable ();
(also note VAR_P).  And, please use hasn%'t instead of hasn't.

> @@ -1153,9 +1184,8 @@ input_overwrite_node (struct lto_file_decl_data 
> *file_data,
>node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution,
>LDPR_NUM_KNOWN);
>node->split_part = bp_unpack_value (bp, 1);
> -  gcc_assert (flag_ltrans
> -   || (!node->in_other_partition
> -   && !node->used_from_other_partition));
> +  verify_node_partition (node->decl, node->name (), node->in_other_partition,
> +  node->used_from_other_partition);
>  }

Why are you passing all these arguments to that function (especially calling
node->name () even when you don't know if it will be needed or not)?
Doesn't both cgraph_node and varpool_node inherit from symtab_node, which
has all these 3 fields as well as name () method?
So, I think if verify_node_partition takes a symtab_node *node argument
and the callers just both do
  verify_node_partition (node);
it should work fine.  I'd make it inline too.

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/function-not-offloaded.c
> @@ -0,0 +1,21 @@
> +/* { dg-do link } */
> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
> +
> +#pragma omp declare target
> +int results[2000];
> +#pragma omp end declare target
> +
> +void __attribute__((noinline, noclone))
> +baz (int i) /* { dg-error "function 'baz' has been referenced in offloaded 
> code but hasn't been marked to be included in the offloaded code" } */
> +{
> +  results[i]++;
> +}
> +
> +int
> +main ()
> +{
> +#pragma omp target
> +#pragma omp for
> +  for (int i = 0; i < 2000; i++)
> +baz (i);
> +}

Note, this will be well defined in OpenMP 5.0, just the support isn't there.
The spec says:
"If a function (C, C++, Fortran) or subroutine (Fortran) is referenced in a 
target construct then
that function or subroutine is treated as if its name had appeared in a to 
clause on a
declare target directive."
and
"If a function is referenced in a function that appears as a list item in a to 
clause on a
declare target directive then the name of the referenced function is treated as 
if it had
appeared in a to clause on a declare target directive.

If a variable with static storage duration or a function (except lambda for 
C++) is referenced in the
initializer expression list of a variable with static storage duration that 
appears as a list item in a to
clause on a declare target directive then the name of the referenced variable 
or function is
treated as if it had appeared in a to clause on a declare target directive."

so for functions it should work through implicit propagation of the
"omp declare target" attribute.

Can't find a restriction I'd expect to see that if a declaration of a
function or variable is marked declare target then the definition has to be
as well, will talk to omp-lang.

In any case, for the above testcase it might be better to split it into two
sources with dg-auxiliary-source, declare baz in the source with main and
define in another file (where declare instead of define the variable).

> diff --git a/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c 
> b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
> new file mode 100644
> index 000..c2e1d57adea
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/variable-not-offloaded.c
> @@ -0,0 +1,21 @@
> +/* { dg-do link } */
> +/* { dg-excess-errors "lto1, mkoffload and lto-wrapper fatal errors" } */
> +
> +int results[2000]; /* { dg-error "variable 'results' has been referenced in 
> offloaded code but hasn't been marked to be included in the offloaded code" } 
> */
> +
> +#pragma omp declare target
> +void  __attribute__((noinline, noclone))
> +baz (int i)
> +{
> +  results[i]++;
> +}
> +#pragma omp end declare target
> +
> +int
> +main ()
> +{
> +#pragma omp target
> +#pragma omp for
> 

Re: [PATCH] error on missing LTO symbols

[Tom de Vries]

On 13-12-18 14:44, Jakub Jelinek wrote:
> On Thu, Dec 13, 2018 at 02:31:45PM +0100, Tom de Vries wrote:
>>> 2015-07-24  Cesar Philippidis  
> 
> Please use current date ;)
> 
>>>
>>> gcc/
>>> * lto-cgraph.c (input_overwrite_node): Error instead of assert
>>> on missing cgraph partitions.
>>> (input_varpool_node): Likewise.
>>>
>>>
>>> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
>>> index d70537d..7e2fc80 100644
>>> --- a/gcc/lto-cgraph.c
>>> +++ b/gcc/lto-cgraph.c
>>> @@ -1218,9 +1218,11 @@ input_overwrite_node (struct lto_file_decl_data
>>> *file_data,
>>>  LDPR_NUM_KNOWN);
>>>node->instrumentation_clone = bp_unpack_value (bp, 1);
>>>node->split_part = bp_unpack_value (bp, 1);
>>> -  gcc_assert (flag_ltrans
>>> - || (!node->in_other_partition
>>> - && !node->used_from_other_partition));
>>> +
>>> +  int success = flag_ltrans || (!node->in_other_partition
>>> +   && !node->used_from_other_partition);
> 
> I'd use internal_error (or internal_error_no_backtrace ?) here if it isn't
> an offloading compiler (so #ifndef ACCEL_COMPILER), or just gcc_assert in
> that case?  Richard/Honza, your thoughts on that?

Left it as gcc_assert for now.

> And error if ACCEL_COMPILER is defined.
> 

Done.

>>> +  if (!success)
>>> +error ("Missing %<%s%>", node->name ());
> 
> Diagnostics shouldn't start with capital letters.  %<%s%> should be %qs.
> 

Done.

> The diagnostics for the non-accel case if any can be less verbose, but
> perhaps should at least say whether it is a function (above case) or
> variable.
> 

I've left the non-accel case alone for now.

> For the ACCEL_COMPILER case, I think we should be more verbose, say that
> certain function or variable has been referenced in offloaded code but
> corresponding function or variable definition hasn't been marked to be
> included in the offloaded code.

Done.

> Would be nice to have a location where it has been referenced if
> that can be dug from somewhere.

It's not obvious to me how to do that, so I've used the location of the
declaration of the symbol instead.

> Or if e.g. from attribute we can figure out
> whether it was OpenMP or OpenACC offloading and even suggest how to fix it
> in the corresponding language extension.

I left it language-agnostic for now.

OK for trunk?

Thanks,
- Tom
[offloading] Error on missing symbols

When compiling an OpenMP or OpenACC program containing a reference in the
offloaded code to a symbol that has not been included in the offloaded code,
the offloading compiler may ICE in lto1.

Fix this by erroring out instead, mentioning the problematic symbol:
...
error: variable 'results' has been referenced in offloaded code but hasn't
  been marked to be included in the offloaded code
lto1: fatal error: errors during merging of translation units
compilation terminated.
...

Build and reg-tested on x86_64 with nvptx accelerator.

2018-12-13  Tom de Vries  

	* lto-cgraph.c (verify_node_partition): New function.
	(input_overwrite_node, input_varpool_node): Use verify_node_partition.

	* testsuite/libgomp.c-c++-common/function-not-offloaded.c: New test.
	* testsuite/libgomp.c-c++-common/variable-not-offloaded.c: New test.

---
 gcc/lto-cgraph.c   | 42 ++
 .../libgomp.c-c++-common/function-not-offloaded.c  | 21 +++
 .../libgomp.c-c++-common/variable-not-offloaded.c  | 21 +++
 3 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index 8cc3c75..f10a96bd2a4 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1091,6 +1091,37 @@ output_offload_tables (void)
 }
 }
 
+/* Verify the partitioning of a varpool_node or cgraph_node with DECL and NAME,
+   as specified by IN_OTHER_PARTITION and USED_FROM_OTHER_PARTITION.  */
+
+static void
+verify_node_partition (tree decl ATTRIBUTE_UNUSED,
+		   const char *name ATTRIBUTE_UNUSED,
+		   bool in_other_partition,
+		   bool used_from_other_partition ATTRIBUTE_UNUSED)
+{
+  if (flag_ltrans)
+return;
+
+#ifdef ACCEL_COMPILER
+  if (in_other_partition)
+{
+  if (TREE_CODE (decl) == FUNCTION_DECL
+	  || TREE_CODE (decl) == VAR_DECL)
+	error_at (DECL_SOURCE_LOCATION (decl),
+		  "%s %qs has been referenced in offloaded code but"
+		  " hasn't been marked to be included in the offloaded code",
+		  TREE_CODE (decl) == FUNCTION_DECL ? "function" : "variable",
+		  name);
+  else
+	gcc_unreachable ();
+}
+#else
+  gcc_assert (!in_other_partition
+	  && !used_from_other_partition);
+#endif
+}
+
 /* Overwrite the information in NODE based on FILE_DATA, TAG, FLAGS,
STACK_SIZE, SELF_TIME and SELF_SIZE.  This is called either to initialize
NODE or to replace the values in it, for instance because the first
@@ -1153,9 +1184,8 @@ input_overwrite_node (struct lto_file_decl_data *file_data,
   node->resolution = bp_unpack_enum (bp, 

Re: [PATCH] error on missing LTO symbols

[Jakub Jelinek]

On Thu, Dec 13, 2018 at 02:31:45PM +0100, Tom de Vries wrote:
> > 2015-07-24  Cesar Philippidis  

Please use current date ;)

> > 
> > gcc/
> > * lto-cgraph.c (input_overwrite_node): Error instead of assert
> > on missing cgraph partitions.
> > (input_varpool_node): Likewise.
> > 
> > 
> > diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
> > index d70537d..7e2fc80 100644
> > --- a/gcc/lto-cgraph.c
> > +++ b/gcc/lto-cgraph.c
> > @@ -1218,9 +1218,11 @@ input_overwrite_node (struct lto_file_decl_data
> > *file_data,
> >  LDPR_NUM_KNOWN);
> >node->instrumentation_clone = bp_unpack_value (bp, 1);
> >node->split_part = bp_unpack_value (bp, 1);
> > -  gcc_assert (flag_ltrans
> > - || (!node->in_other_partition
> > - && !node->used_from_other_partition));
> > +
> > +  int success = flag_ltrans || (!node->in_other_partition
> > +   && !node->used_from_other_partition);

I'd use internal_error (or internal_error_no_backtrace ?) here if it isn't
an offloading compiler (so #ifndef ACCEL_COMPILER), or just gcc_assert in
that case?  Richard/Honza, your thoughts on that?
And error if ACCEL_COMPILER is defined.

> > +  if (!success)
> > +error ("Missing %<%s%>", node->name ());

Diagnostics shouldn't start with capital letters.  %<%s%> should be %qs.

The diagnostics for the non-accel case if any can be less verbose, but
perhaps should at least say whether it is a function (above case) or
variable.

For the ACCEL_COMPILER case, I think we should be more verbose, say that
certain function or variable has been referenced in offloaded code but
corresponding function or variable definition hasn't been marked to be
included in the offloaded code.
Would be nice to have a location where it has been referenced if
that can be dug from somewhere.  Or if e.g. from attribute we can figure out
whether it was OpenMP or OpenACC offloading and even suggest how to fix it
in the corresponding language extension.

Jakub

Re: [PATCH] error on missing LTO symbols

[Tom de Vries]

[ adding gcc-patches ]

On 13-12-18 14:30, Tom de Vries wrote:
> [ Patch copy-pasted from here (
> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02076.html ). Patch was
> resubmitted here (
> https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00043.html ). ]
> 
> Hi,
> 
> this issue just popped up again for PR88460: an openmp test-case problem
> manifests as an ICE. We've seen the same problem for openacc test-cases.
> 
> From my point of view, the main problem with the current state is that
> we don't known what symbol the ICE relates to, and can only find out by
> reproducing the compile in a debugging session and doing "call
> debug_generic_expr (node.decl)" ( see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88460#c0 ), which is
> cumbersome for gcc developers, and not helpful for openmp and openacc users.
> 
> This patch attempts to address that issue by changing the assert into an
> error. [ We could make the error message more specific, by
> distinguishing between the node->in_other_partition and
> node->used_from_other_partition cases. ]
> 
> Is the approach in this patch OK (perhaps conditionalizing it to be only
> effective for openacc and openmp)?
> 
> Alternatively, we could instead print a message ahead of the ICE.
> 
> Can we have some feedback on what would be an acceptable solution here?
> 
> Thanks,
> - Tom
> 
> 2015-07-24  Cesar Philippidis  
> 
>   gcc/
>   * lto-cgraph.c (input_overwrite_node): Error instead of assert
>   on missing cgraph partitions.
>   (input_varpool_node): Likewise.
> 
> 
> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
> index d70537d..7e2fc80 100644
> --- a/gcc/lto-cgraph.c
> +++ b/gcc/lto-cgraph.c
> @@ -1218,9 +1218,11 @@ input_overwrite_node (struct lto_file_decl_data
> *file_data,
>LDPR_NUM_KNOWN);
>node->instrumentation_clone = bp_unpack_value (bp, 1);
>node->split_part = bp_unpack_value (bp, 1);
> -  gcc_assert (flag_ltrans
> -   || (!node->in_other_partition
> -   && !node->used_from_other_partition));
> +
> +  int success = flag_ltrans || (!node->in_other_partition
> + && !node->used_from_other_partition);
> +  if (!success)
> +error ("Missing %<%s%>", node->name ());
>  }
> 
>  /* Return string alias is alias of.  */
> @@ -1432,9 +1434,11 @@ input_varpool_node (struct lto_file_decl_data
> *file_data,
>  node->set_section_for_node (section);
>node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution,
>   LDPR_NUM_KNOWN);
> -  gcc_assert (flag_ltrans
> -   || (!node->in_other_partition
> -   && !node->used_from_other_partition));
> +
> +  int success = flag_ltrans || (!node->in_other_partition
> + && !node->used_from_other_partition);
> +  if (!success)
> +error ("Missing %<%s%>", node->name ());
> 
>return node;
>  }
> 

[PATCH] error on missing LTO symbols

[Cesar Philippidis]

Both OpenMP and OpenACC allow the user to call functions and access
global variables. Usually, the user needs to explicitly mark those
functions and variables as offloadable. However, for certain functions,
such as those in libc, libm, etc., it makes sense to allow the user call
functions and use variables that haven't been marked as offloadable.
This patch teaches the lto streamer how to treat missing symbols as
errors instead of assertion failures.

Is this patch OK for trunk and gcc6? Or should we keep the assertion
failure when -fopenacc, -fopenmp, or -fopenmp-simd isn't set?

This patch was originally posted last year:
. I was trying
to avoid it for OpenACC, but making non-acc routine calls errors would
complicate library functions.

Cesar
2016-07-01  Cesar Philippidis  

	gcc/
	* lto-cgraph.c (input_overwrite_node): Change the assertion to an
	error for missing symbols.
	(input_varpool_node): Likewise.


diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index 5cef2ba..552ea6b 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1201,9 +1201,11 @@ input_overwrite_node (struct lto_file_decl_data *file_data,
  LDPR_NUM_KNOWN);
   node->instrumentation_clone = bp_unpack_value (bp, 1);
   node->split_part = bp_unpack_value (bp, 1);
-  gcc_assert (flag_ltrans
-	  || (!node->in_other_partition
-		  && !node->used_from_other_partition));
+
+  int success = flag_ltrans || (!node->in_other_partition
+&& !node->used_from_other_partition);
+  if (!success)
+error ("Missing %<%s%>", node->name ());
 }
 
 /* Return string alias is alias of.  */
@@ -1416,9 +1418,11 @@ input_varpool_node (struct lto_file_decl_data *file_data,
 node->set_section_for_node (section);
   node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution,
 	LDPR_NUM_KNOWN);
-  gcc_assert (flag_ltrans
-	  || (!node->in_other_partition
-		  && !node->used_from_other_partition));
+
+  int success = flag_ltrans || (!node->in_other_partition
+&& !node->used_from_other_partition);
+  if (!success)
+error ("Missing %<%s%>", node->name ());
 
   return node;
 }