Re: [PATCH] Support bitfields in Wodr machinery (PR lto/85405).

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 19 April 2018 at 10:37, Martin Liška <mliska@suse.cz> wrote:
> On 04/18/2018 10:51 PM, Christophe Lyon wrote:
>> Hi,
>>
>>
>> On 17 April 2018 at 10:19, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> On 04/17/2018 08:58 AM, Jakub Jelinek wrote:
>>>>> On Tue, Apr 17, 2018 at 07:39:20AM +0200, Martin Liška wrote:
>>>>>> +          if (DECL_BIT_FIELD (f1) != DECL_BIT_FIELD (f2))
>>>>>> +            {
>>>>>> +              warn_odr (t1, t2, f1, f2, warn, warned,
>>>>>> +                        G_ ("one field is bitfield while other is not "));
>>>>>
>>>>> I think all the G_ uses don't put a space in between G_ and (
>>>>> Also, please avoid the trailing space in the message.
>>>>
>>>> Sure. I see other format violations, should I fix that in follow up patch:
>>>>
>>>> gcc/c-family/c-warn.c:           ? G_ ("floating point overflow in expression %qE "
>>>> gcc/c-family/c-warn.c:           : G_ ("floating point overflow in expression of type %qT "
>>>> gcc/gimple-ssa-sprintf.c:                         ? G_ ("%<%.*s%> directive output between %wu and "
>>>> gcc/gimple-ssa-sprintf.c:                         : G_ ("%<%.*s%> directive output between %wu and "
>>>> gcc/testsuite/gcc.dg/plugin/ggcplug.c:      warning (0, G_ ("option '-fplugin-arg-%s-count-ggc-start=%s'"
>>>> gcc/testsuite/gcc.dg/plugin/ggcplug.c:      warning (0, G_ ("option '-fplugin-arg-%s-count-ggc-end=%s'"
>>>> gcc/testsuite/gcc.dg/plugin/ggcplug.c:      warning (0, G_ ("option '-fplugin-arg-%s-count-ggc-mark=%s'"
>>>> gcc/testsuite/gcc.dg/plugin/ggcplug.c:      warning (0, G_ ("option '-fplugin-arg-%s-test-extra-root=%s'"
>>>>
>>>> ?
>>>>
>>>>>
>>>>> Do you diagnose if both are bit-fields, but with different bitcount, e.g. if
>>>>> in your testcase you replace bool mbDisposed : 1; with int mbDisposed : 3;
>>>>> and bool mbDisposed; with int mbDisposed : 7; ?
>>>>
>>>> Good point, I add a new test-case, done in patch.
>>>> Is it OK to install the patch?
>>> OK, thanks! (and sorry for the whitespace errors)
>>> Honza
>>>>
>>>> Martin
>>>>
>>>>>
>>>>>> +              return false;
>>>>>> +            }
>>>>>> +          else
>>>>>> +            gcc_assert (DECL_NONADDRESSABLE_P (f1)
>>>>>> +                        == DECL_NONADDRESSABLE_P (f2));
>>>>>>          }
>>>>>>
>>>>>>        /* If one aggregate has more fields than the other, they
>>>>>> diff --git a/gcc/testsuite/g++.dg/lto/pr85405_0.C b/gcc/testsuite/g++.dg/lto/pr85405_0.C
>>>>>> new file mode 100644
>>>>>> index 00000000000..1a41d81099c
>>>>>> --- /dev/null
>>>>>> +++ b/gcc/testsuite/g++.dg/lto/pr85405_0.C
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +// { dg-lto-do link }
>>>>>> +// { dg-lto-options {{-fPIC -shared -flto}} }
>>>>>> +
>>>>>> +class VclReferenceBase { // { dg-lto-warning "7: type 'struct VclReferenceBase' violates the C\\+\\+ One Definition Rule" }
>>>>>> +  int mnRefCnt;
>>>>>> +  bool mbDisposed : 1;
>>>>>> +  virtual ~VclReferenceBase();
>>>>>> +};
>>>>>> +class a;
>>>>>> +class b {
>>>>>> +  a &e;
>>>>>> +  bool c();
>>>>>> +};
>>>>>> +class B {
>>>>>> +  VclReferenceBase d;
>>>>>> +};
>>>>>> +class a : B {};
>>>>>> +bool b::c() { return false; }
>>>>>> diff --git a/gcc/testsuite/g++.dg/lto/pr85405_1.C b/gcc/testsuite/g++.dg/lto/pr85405_1.C
>>>>>> new file mode 100644
>>>>>> index 00000000000..78606185624
>>>>>> --- /dev/null
>>>>>> +++ b/gcc/testsuite/g++.dg/lto/pr85405_1.C
>>>>>> @@ -0,0 +1,9 @@
>>>>>> +class VclReferenceBase {
>>>>>> +  int mnRefCnt;
>>>>>> +  bool mbDisposed;
>>>>>> +
>>>>>> +protected:
>>>>>> +  virtual ~VclReferenceBase();
>>>>>> +};
>>>>>> +class : VclReferenceBase {
>>>>>> +} a;
>>>>>>
>>>>>
>>>>>     Jakub
>>>>>
>>>>
>>>
>>>> From 87380235f9b81bea4cf910e702192586c072ce93 Mon Sep 17 00:00:00 2001
>>>> From: marxin <mliska@suse.cz>
>>>> Date: Tue, 17 Apr 2018 10:11:07 +0200
>>>> Subject: [PATCH] Fix coding style and add a new test-case (PR lto/85405).
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2018-04-17  Martin Liska  <mliska@suse.cz>
>>>>
>>>>       PR lto/85405
>>>>       * ipa-devirt.c (odr_types_equivalent_p):
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2018-04-17  Martin Liska  <mliska@suse.cz>
>>>>
>>>>       PR lto/85405
>>>>       * g++.dg/lto/pr85405b_0.C: New test.
>>>>       * g++.dg/lto/pr85405b_1.C: New test.
>>
>> The new testcases require that -shared and -fpic work, which is not
>> the case on bare-metal targets
>> (eg arm-eabi, aarch64-elf).
>>
>> The attached small patch adds
>> +// { dg-require-effective-target shared }
>> +// { dg-require-effective-target fpic }
>> so the tests are skipped on such targets.
>>
>> OK for trunk?
>
> Thanks for the fix. It's quite obvious, please install it.
>

Thanks, done.

I also patched gcc/testsuite/g++.dg/lto/pr84805_0.C in the same way.

Christophe

> Martin
>
>>
>> Thanks,
>>
>> Christophe
>>
>>>> ---
>>>>  gcc/ipa-devirt.c                      |  2 +-
>>>>  gcc/testsuite/g++.dg/lto/pr85405b_0.C | 18 ++++++++++++++++++
>>>>  gcc/testsuite/g++.dg/lto/pr85405b_1.C |  9 +++++++++
>>>>  3 files changed, 28 insertions(+), 1 deletion(-)
>>>>  create mode 100644 gcc/testsuite/g++.dg/lto/pr85405b_0.C
>>>>  create mode 100644 gcc/testsuite/g++.dg/lto/pr85405b_1.C
>>>>
>>>> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
>>>> index 85b8ef175f3..cc9b5e347e6 100644
>>>> --- a/gcc/ipa-devirt.c
>>>> +++ b/gcc/ipa-devirt.c
>>>> @@ -1599,7 +1599,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>               if (DECL_BIT_FIELD (f1) != DECL_BIT_FIELD (f2))
>>>>                 {
>>>>                   warn_odr (t1, t2, f1, f2, warn, warned,
>>>> -                           G_ ("one field is bitfield while other is not "));
>>>> +                           G_("one field is bitfield while other is not"));
>>>>                   return false;
>>>>                 }
>>>>               else
>>>> diff --git a/gcc/testsuite/g++.dg/lto/pr85405b_0.C b/gcc/testsuite/g++.dg/lto/pr85405b_0.C
>>>> new file mode 100644
>>>> index 00000000000..a692abb7715
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/g++.dg/lto/pr85405b_0.C
>>>> @@ -0,0 +1,18 @@
>>>> +// { dg-lto-do link }
>>>> +// { dg-lto-options {{-fPIC -shared -flto}} }
>>>> +
>>>> +class VclReferenceBase { // { dg-lto-warning "7: type 'struct VclReferenceBase' violates the C\\+\\+ One Definition Rule" }
>>>> +  int mnRefCnt;
>>>> +  int mbDisposed : 3;
>>>> +  virtual ~VclReferenceBase();
>>>> +};
>>>> +class a;
>>>> +class b {
>>>> +  a &e;
>>>> +  bool c();
>>>> +};
>>>> +class B {
>>>> +  VclReferenceBase d;
>>>> +};
>>>> +class a : B {};
>>>> +bool b::c() { return false; }
>>>> diff --git a/gcc/testsuite/g++.dg/lto/pr85405b_1.C b/gcc/testsuite/g++.dg/lto/pr85405b_1.C
>>>> new file mode 100644
>>>> index 00000000000..fd98e631d56
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/g++.dg/lto/pr85405b_1.C
>>>> @@ -0,0 +1,9 @@
>>>> +class VclReferenceBase {
>>>> +  int mnRefCnt;
>>>> +  int mbDisposed: 7; // { dg-lto-message "19: a field of same name but different type is defined in another translation unit" }
>>>> +
>>>> +protected:
>>>> +  virtual ~VclReferenceBase();
>>>> +};
>>>> +class : VclReferenceBase {
>>>> +} a;
>>>> --
>>>> 2.16.3
>>>>
>>>
>