[PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[Xi Ruoyao]

We are comparing enum values (in wide_int) to check ODR violation.
However, if we compare two wide_int values with different precision,
we'll trigger an assert, leading to ICE.  With enum-base introduced
in C++11, it's easy to sink into this situation.

To fix the issue, we need to explicitly check this kind of mismatch,
and emit a proper warning message if there is such one.

Bootstrapped & regtested on x86_64-linux-gnu.  Ok for trunk?

gcc/

	PR ipa/101396
	* ipa-devirt.c (ipa_odr_read_section): Compare the precision of
	  enum values, and emit a warning if they mismatch.

gcc/testsuite/

	PR ipa/101396
	* g++.dg/lto/pr101396_0.C: New test.
	* g++.dg/lto/pr101396_1.C: New test.
---
 gcc/ipa-devirt.c                      |  9 +++++++++
 gcc/testsuite/g++.dg/lto/pr101396_0.C | 12 ++++++++++++
 gcc/testsuite/g++.dg/lto/pr101396_1.C | 10 ++++++++++
 3 files changed, 31 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_0.C
 create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_1.C

diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 8cd1100aba9..8deec75b2df 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -4193,6 +4193,8 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data,
 	      if (do_warning != -1 || j >= this_enum.vals.length ())
 		continue;
 	      if (strcmp (id, this_enum.vals[j].name)
+		  || (val.get_precision() !=
+		      this_enum.vals[j].val.get_precision())
 		  || val != this_enum.vals[j].val)
 		{
 		  warn_name = xstrdup (id);
@@ -4260,6 +4262,13 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data,
 			    "name %qs differs from name %qs defined"
 			    " in another translation unit",
 			    this_enum.vals[j].name, warn_name);
+		  else if (this_enum.vals[j].val.get_precision() !=
+			   warn_value.get_precision())
+		    inform (this_enum.vals[j].locus,
+			    "name %qs is defined as %u-bit while another "
+			    "translation unit defines it as %u-bit",
+			    warn_name, this_enum.vals[j].val.get_precision(),
+			    warn_value.get_precision());
 		  /* FIXME: In case there is easy way to print wide_ints,
 		     perhaps we could do it here instead of overflow check.  */
 		  else if (wi::fits_shwi_p (this_enum.vals[j].val)
diff --git a/gcc/testsuite/g++.dg/lto/pr101396_0.C b/gcc/testsuite/g++.dg/lto/pr101396_0.C
new file mode 100644
index 00000000000..b7a2947a880
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr101396_0.C
@@ -0,0 +1,12 @@
+/* { dg-lto-do link } */
+
+enum A : __UINT32_TYPE__ { // { dg-lto-warning "6: type 'A' violates the C\\+\\+ One Definition Rule" }
+  a, // { dg-lto-note "3: name 'a' is defined as 32-bit while another translation unit defines it as 64-bit" }
+  b,
+  c
+};
+
+int main()
+{
+  return (int) A::a;
+}
diff --git a/gcc/testsuite/g++.dg/lto/pr101396_1.C b/gcc/testsuite/g++.dg/lto/pr101396_1.C
new file mode 100644
index 00000000000..a6d032d694d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr101396_1.C
@@ -0,0 +1,10 @@
+enum A : __UINT64_TYPE__ { // { dg-lto-note "6: an enum with different value name is defined in another translation unit" }
+  a, // { dg-lto-note "3: mismatching definition" }
+  b,
+  c
+};
+
+int f(enum A x)
+{
+  return (int) x;
+}
-- 
2.32.0

PING^1: [PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[Xi Ruoyao]

Ping.

On Sun, 2021-07-11 at 01:48 +0800, Xi Ruoyao wrote:
> We are comparing enum values (in wide_int) to check ODR violation.
> However, if we compare two wide_int values with different precision,
> we'll trigger an assert, leading to ICE.  With enum-base introduced
> in C++11, it's easy to sink into this situation.
> 
> To fix the issue, we need to explicitly check this kind of mismatch,
> and emit a proper warning message if there is such one.
> 
> Bootstrapped & regtested on x86_64-linux-gnu.  Ok for trunk?
> 
> gcc/
> 
>         PR ipa/101396
>         * ipa-devirt.c (ipa_odr_read_section): Compare the precision of
>           enum values, and emit a warning if they mismatch.
> 
> gcc/testsuite/
> 
>         PR ipa/101396
>         * g++.dg/lto/pr101396_0.C: New test.
>         * g++.dg/lto/pr101396_1.C: New test.
> ---
>  gcc/ipa-devirt.c                      |  9 +++++++++
>  gcc/testsuite/g++.dg/lto/pr101396_0.C | 12 ++++++++++++
>  gcc/testsuite/g++.dg/lto/pr101396_1.C | 10 ++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_0.C
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_1.C
> 
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 8cd1100aba9..8deec75b2df 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -4193,6 +4193,8 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data,
>               if (do_warning != -1 || j >= this_enum.vals.length ())
>                 continue;
>               if (strcmp (id, this_enum.vals[j].name)
> +                 || (val.get_precision() !=
> +                     this_enum.vals[j].val.get_precision())
>                   || val != this_enum.vals[j].val)
>                 {
>                   warn_name = xstrdup (id);
> @@ -4260,6 +4262,13 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data,
>                             "name %qs differs from name %qs defined"
>                             " in another translation unit",
>                             this_enum.vals[j].name, warn_name);
> +                 else if (this_enum.vals[j].val.get_precision() !=
> +                          warn_value.get_precision())
> +                   inform (this_enum.vals[j].locus,
> +                           "name %qs is defined as %u-bit while another "
> +                           "translation unit defines it as %u-bit",
> +                           warn_name, this_enum.vals[j].val.get_precision(),
> +                           warn_value.get_precision());
>                   /* FIXME: In case there is easy way to print wide_ints,
>                      perhaps we could do it here instead of overflow check.  */
>                   else if (wi::fits_shwi_p (this_enum.vals[j].val)
> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_0.C b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> new file mode 100644
> index 00000000000..b7a2947a880
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> @@ -0,0 +1,12 @@
> +/* { dg-lto-do link } */
> +
> +enum A : __UINT32_TYPE__ { // { dg-lto-warning "6: type 'A' violates the C\\+\\+ One Definition Rule" }
> +  a, // { dg-lto-note "3: name 'a' is defined as 32-bit while another translation unit defines it as 64-bit" }
> +  b,
> +  c
> +};
> +
> +int main()
> +{
> +  return (int) A::a;
> +}
> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_1.C b/gcc/testsuite/g++.dg/lto/pr101396_1.C
> new file mode 100644
> index 00000000000..a6d032d694d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr101396_1.C
> @@ -0,0 +1,10 @@
> +enum A : __UINT64_TYPE__ { // { dg-lto-note "6: an enum with different value name is defined in another translation unit" }
> +  a, // { dg-lto-note "3: mismatching definition" }
> +  b,
> +  c
> +};
> +
> +int f(enum A x)
> +{
> +  return (int) x;
> +}

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[Richard Sandiford]

Xi Ruoyao via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> We are comparing enum values (in wide_int) to check ODR violation.
> However, if we compare two wide_int values with different precision,
> we'll trigger an assert, leading to ICE.  With enum-base introduced
> in C++11, it's easy to sink into this situation.
>
> To fix the issue, we need to explicitly check this kind of mismatch,
> and emit a proper warning message if there is such one.
>
> Bootstrapped & regtested on x86_64-linux-gnu.  Ok for trunk?
>
> gcc/
>
> 	PR ipa/101396
> 	* ipa-devirt.c (ipa_odr_read_section): Compare the precision of
> 	  enum values, and emit a warning if they mismatch.

OK, thanks.

Richard

> gcc/testsuite/
>
> 	PR ipa/101396
> 	* g++.dg/lto/pr101396_0.C: New test.
> 	* g++.dg/lto/pr101396_1.C: New test.
> ---
>  gcc/ipa-devirt.c                      |  9 +++++++++
>  gcc/testsuite/g++.dg/lto/pr101396_0.C | 12 ++++++++++++
>  gcc/testsuite/g++.dg/lto/pr101396_1.C | 10 ++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_0.C
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_1.C
>
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 8cd1100aba9..8deec75b2df 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -4193,6 +4193,8 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data,
>  	      if (do_warning != -1 || j >= this_enum.vals.length ())
>  		continue;
>  	      if (strcmp (id, this_enum.vals[j].name)
> +		  || (val.get_precision() !=
> +		      this_enum.vals[j].val.get_precision())
>  		  || val != this_enum.vals[j].val)
>  		{
>  		  warn_name = xstrdup (id);
> @@ -4260,6 +4262,13 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data,
>  			    "name %qs differs from name %qs defined"
>  			    " in another translation unit",
>  			    this_enum.vals[j].name, warn_name);
> +		  else if (this_enum.vals[j].val.get_precision() !=
> +			   warn_value.get_precision())
> +		    inform (this_enum.vals[j].locus,
> +			    "name %qs is defined as %u-bit while another "
> +			    "translation unit defines it as %u-bit",
> +			    warn_name, this_enum.vals[j].val.get_precision(),
> +			    warn_value.get_precision());
>  		  /* FIXME: In case there is easy way to print wide_ints,
>  		     perhaps we could do it here instead of overflow check.  */
>  		  else if (wi::fits_shwi_p (this_enum.vals[j].val)
> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_0.C b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> new file mode 100644
> index 00000000000..b7a2947a880
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> @@ -0,0 +1,12 @@
> +/* { dg-lto-do link } */
> +
> +enum A : __UINT32_TYPE__ { // { dg-lto-warning "6: type 'A' violates the C\\+\\+ One Definition Rule" }
> +  a, // { dg-lto-note "3: name 'a' is defined as 32-bit while another translation unit defines it as 64-bit" }
> +  b,
> +  c
> +};
> +
> +int main()
> +{
> +  return (int) A::a;
> +}
> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_1.C b/gcc/testsuite/g++.dg/lto/pr101396_1.C
> new file mode 100644
> index 00000000000..a6d032d694d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr101396_1.C
> @@ -0,0 +1,10 @@
> +enum A : __UINT64_TYPE__ { // { dg-lto-note "6: an enum with different value name is defined in another translation unit" }
> +  a, // { dg-lto-note "3: mismatching definition" }
> +  b,
> +  c
> +};
> +
> +int f(enum A x)
> +{
> +  return (int) x;
> +}

PING^w: [PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[Xi Ruoyao]

Ping again.

On Sun, 2021-07-11 at 01:48 +0800, Xi Ruoyao wrote:
> We are comparing enum values (in wide_int) to check ODR violation.
> However, if we compare two wide_int values with different precision,
> we'll trigger an assert, leading to ICE.  With enum-base introduced
> in C++11, it's easy to sink into this situation.
> 
> To fix the issue, we need to explicitly check this kind of mismatch,
> and emit a proper warning message if there is such one.
> 
> Bootstrapped & regtested on x86_64-linux-gnu.  Ok for trunk?
> 
> gcc/
> 
>         PR ipa/101396
>         * ipa-devirt.c (ipa_odr_read_section): Compare the precision
> of
>           enum values, and emit a warning if they mismatch.
> 
> gcc/testsuite/
> 
>         PR ipa/101396
>         * g++.dg/lto/pr101396_0.C: New test.
>         * g++.dg/lto/pr101396_1.C: New test.
> ---
>  gcc/ipa-devirt.c                      |  9 +++++++++
>  gcc/testsuite/g++.dg/lto/pr101396_0.C | 12 ++++++++++++
>  gcc/testsuite/g++.dg/lto/pr101396_1.C | 10 ++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_0.C
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_1.C
> 
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 8cd1100aba9..8deec75b2df 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -4193,6 +4193,8 @@ ipa_odr_read_section (struct lto_file_decl_data
> *file_data, const char *data,
>               if (do_warning != -1 || j >= this_enum.vals.length ())
>                 continue;
>               if (strcmp (id, this_enum.vals[j].name)
> +                 || (val.get_precision() !=
> +                     this_enum.vals[j].val.get_precision())
>                   || val != this_enum.vals[j].val)
>                 {
>                   warn_name = xstrdup (id);
> @@ -4260,6 +4262,13 @@ ipa_odr_read_section (struct lto_file_decl_data
> *file_data, const char *data,
>                             "name %qs differs from name %qs defined"
>                             " in another translation unit",
>                             this_enum.vals[j].name, warn_name);
> +                 else if (this_enum.vals[j].val.get_precision() !=
> +                          warn_value.get_precision())
> +                   inform (this_enum.vals[j].locus,
> +                           "name %qs is defined as %u-bit while
> another "
> +                           "translation unit defines it as %u-bit",
> +                           warn_name,
> this_enum.vals[j].val.get_precision(),
> +                           warn_value.get_precision());
>                   /* FIXME: In case there is easy way to print
> wide_ints,
>                      perhaps we could do it here instead of overflow
> check.  */
>                   else if (wi::fits_shwi_p (this_enum.vals[j].val)
> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_0.C
> b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> new file mode 100644
> index 00000000000..b7a2947a880
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> @@ -0,0 +1,12 @@
> +/* { dg-lto-do link } */
> +
> +enum A : __UINT32_TYPE__ { // { dg-lto-warning "6: type 'A' violates
> the C\\+\\+ One Definition Rule" }
> +  a, // { dg-lto-note "3: name 'a' is defined as 32-bit while another
> translation unit defines it as 64-bit" }
> +  b,
> +  c
> +};
> +
> +int main()
> +{
> +  return (int) A::a;
> +}
> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_1.C
> b/gcc/testsuite/g++.dg/lto/pr101396_1.C
> new file mode 100644
> index 00000000000..a6d032d694d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr101396_1.C
> @@ -0,0 +1,10 @@
> +enum A : __UINT64_TYPE__ { // { dg-lto-note "6: an enum with
> different value name is defined in another translation unit" }
> +  a, // { dg-lto-note "3: mismatching definition" }
> +  b,
> +  c
> +};
> +
> +int f(enum A x)
> +{
> +  return (int) x;
> +}


-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: PING^w: [PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[Kewen.Lin]

Hi Ruoyao,

on 2021/7/30 下午12:57, Xi Ruoyao via Gcc-patches wrote:
> Ping again.
> 

This ping-ed patch has been approved by Richard at

https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576001.html

Just chime in as I guess you didn't receive his mail somehow.

BR,
Kewen

> On Sun, 2021-07-11 at 01:48 +0800, Xi Ruoyao wrote:
>> We are comparing enum values (in wide_int) to check ODR violation.
>> However, if we compare two wide_int values with different precision,
>> we'll trigger an assert, leading to ICE.  With enum-base introduced
>> in C++11, it's easy to sink into this situation.
>>
>> To fix the issue, we need to explicitly check this kind of mismatch,
>> and emit a proper warning message if there is such one.
>>
>> Bootstrapped & regtested on x86_64-linux-gnu.  Ok for trunk?
>>
>> gcc/
>>
>>         PR ipa/101396
>>         * ipa-devirt.c (ipa_odr_read_section): Compare the precision
>> of
>>           enum values, and emit a warning if they mismatch.
>>
>> gcc/testsuite/
>>
>>         PR ipa/101396
>>         * g++.dg/lto/pr101396_0.C: New test.
>>         * g++.dg/lto/pr101396_1.C: New test.
>> ---
>>  gcc/ipa-devirt.c                      |  9 +++++++++
>>  gcc/testsuite/g++.dg/lto/pr101396_0.C | 12 ++++++++++++
>>  gcc/testsuite/g++.dg/lto/pr101396_1.C | 10 ++++++++++
>>  3 files changed, 31 insertions(+)
>>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_0.C
>>  create mode 100644 gcc/testsuite/g++.dg/lto/pr101396_1.C
>>
>> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
>> index 8cd1100aba9..8deec75b2df 100644
>> --- a/gcc/ipa-devirt.c
>> +++ b/gcc/ipa-devirt.c
>> @@ -4193,6 +4193,8 @@ ipa_odr_read_section (struct lto_file_decl_data
>> *file_data, const char *data,
>>               if (do_warning != -1 || j >= this_enum.vals.length ())
>>                 continue;
>>               if (strcmp (id, this_enum.vals[j].name)
>> +                 || (val.get_precision() !=
>> +                     this_enum.vals[j].val.get_precision())
>>                   || val != this_enum.vals[j].val)
>>                 {
>>                   warn_name = xstrdup (id);
>> @@ -4260,6 +4262,13 @@ ipa_odr_read_section (struct lto_file_decl_data
>> *file_data, const char *data,
>>                             "name %qs differs from name %qs defined"
>>                             " in another translation unit",
>>                             this_enum.vals[j].name, warn_name);
>> +                 else if (this_enum.vals[j].val.get_precision() !=
>> +                          warn_value.get_precision())
>> +                   inform (this_enum.vals[j].locus,
>> +                           "name %qs is defined as %u-bit while
>> another "
>> +                           "translation unit defines it as %u-bit",
>> +                           warn_name,
>> this_enum.vals[j].val.get_precision(),
>> +                           warn_value.get_precision());
>>                   /* FIXME: In case there is easy way to print
>> wide_ints,
>>                      perhaps we could do it here instead of overflow
>> check.  */
>>                   else if (wi::fits_shwi_p (this_enum.vals[j].val)
>> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_0.C
>> b/gcc/testsuite/g++.dg/lto/pr101396_0.C
>> new file mode 100644
>> index 00000000000..b7a2947a880
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/lto/pr101396_0.C
>> @@ -0,0 +1,12 @@
>> +/* { dg-lto-do link } */
>> +
>> +enum A : __UINT32_TYPE__ { // { dg-lto-warning "6: type 'A' violates
>> the C\\+\\+ One Definition Rule" }
>> +  a, // { dg-lto-note "3: name 'a' is defined as 32-bit while another
>> translation unit defines it as 64-bit" }
>> +  b,
>> +  c
>> +};
>> +
>> +int main()
>> +{
>> +  return (int) A::a;
>> +}
>> diff --git a/gcc/testsuite/g++.dg/lto/pr101396_1.C
>> b/gcc/testsuite/g++.dg/lto/pr101396_1.C
>> new file mode 100644
>> index 00000000000..a6d032d694d
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/lto/pr101396_1.C
>> @@ -0,0 +1,10 @@
>> +enum A : __UINT64_TYPE__ { // { dg-lto-note "6: an enum with
>> different value name is defined in another translation unit" }
>> +  a, // { dg-lto-note "3: mismatching definition" }
>> +  b,
>> +  c
>> +};
>> +
>> +int f(enum A x)
>> +{
>> +  return (int) x;
>> +}
> 

Re: PING^w: [PATCH] ipa-devirt: check precision mismatch of enum values [PR101396]

[Xi Ruoyao]

On Fri, 2021-07-30 at 15:00 +0800, Kewen.Lin wrote:
> Hi Ruoyao,
> 
> on 2021/7/30 下午12:57, Xi Ruoyao via Gcc-patches wrote:
> > Ping again.
> > 
> 
> This ping-ed patch has been approved by Richard at
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576001.html
> 
> Just chime in as I guess you didn't receive his mail somehow.

Hi Kewen,

I guessed I missed some mail because I didn't subscribe to gcc-patches.
Thanks for the reminder!

[PATCH committed] ipa-devirt: check precision mismatch of enum values [PR101396]

[Xi Ruoyao]

On Fri, 2021-07-30 at 15:21 +0800, Xi Ruoyao via Gcc-patches wrote:
> On Fri, 2021-07-30 at 15:00 +0800, Kewen.Lin wrote:
> > Hi Ruoyao,
> > 
> > on 2021/7/30 下午12:57, Xi Ruoyao via Gcc-patches wrote:
> > > Ping again.
> > > 
> > 
> > This ping-ed patch has been approved by Richard at
> > 
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576001.html
> > 
> > Just chime in as I guess you didn't receive his mail somehow.
> 
> Hi Kewen,
> 
> I guessed I missed some mail because I didn't subscribe to gcc-
> patches.
> Thanks for the reminder!
> 

Rebased and bootstrapped & regtested on x86_64-linux-gnu.

Committed at 291416d3.