Re: [Mesa-dev] [PATCH mesa 2/6] nouveau: silence paranoid compiler's -Wclass-memaccess

2018-09-23 Thread Karol Herbst 
yeah, you are right, overlooked that "Target target;" inside the inner
"tex" struct.

On Sat, Sep 22, 2018 at 4:27 PM, Jan Vesely  wrote:
> The warning is correct. In the first case, memset tries to zero "Target"
> object which has a non-trivial constructor and non-trivial copy-constructor.
> The original code is broken in the way it mixes C and C++ initialization and
> the patch only papers over the issue.
> The correct fix would be to provide a proper constructor for structs that
> include instances of C++ classes.
>
> Jan
>
>
> On Sat, Sep 22, 2018 at 6:43 AM Karol Herbst  wrote:
>>
>> yeah, I agree here. Either the code was wrong in the first place,
>> which means it would have to be fixed properly or the warning is
>> wrong. The proper fix here is that GCC should detect itself if it's
>> safe to do or not, otherwise that warning becomes a "might be a
>> problem" thing which doesn't help at all. Either it is wrong, or it
>> isn't. And gcc should be able to know in this case.
>>
>> On Sat, Sep 22, 2018 at 6:07 AM, Ilia Mirkin  wrote:
>> > Based on the various fixes, warning seems bogus -- is the proper
>> > solution -Wno-class-memaccess? (Or however one disables such
>> > things...)
>> >
>> > On Fri, Sep 21, 2018 at 9:50 AM, Eric Engestrom
>> >  wrote:
>> >> Signed-off-by: Eric Engestrom 
>> >> ---
>> >>  src/gallium/drivers/nouveau/codegen/nv50_ir.cpp| 2 +-
>> >>  src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp | 2 +-
>> >>  2 files changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> >> b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> >> index 49425b98b9137058c986..62ebc2d24069b7b5f523 100644
>> >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> >> @@ -905,7 +905,7 @@ Instruction::isCommutationLegal(const Instruction
>> >> *i) const
>> >>  TexInstruction::TexInstruction(Function *fn, operation op)
>> >> : Instruction(fn, op, TYPE_F32)
>> >>  {
>> >> -   memset(, 0, sizeof(tex));
>> >> +   memset(static_cast(), 0, sizeof(tex));
>> >>
>> >> tex.rIndirectSrc = -1;
>> >> tex.sIndirectSrc = -1;
>> >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> >> b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> >> index 9193a01f189874a7fb38..b6b9b42964bec670079c 100644
>> >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> >> @@ -454,7 +454,7 @@ CodeEmitter::addInterp(int ipa, int reg, FixupApply
>> >> apply)
>> >>if (!fixupInfo)
>> >>   return false;
>> >>if (n == 0)
>> >> - memset(fixupInfo, 0, sizeof(FixupInfo));
>> >> + memset(static_cast(fixupInfo), 0, sizeof(FixupInfo));
>> >> }
>> >> ++fixupInfo->count;
>> >>
>> >> --
>> >> Cheers,
>> >>   Eric
>> >>
>> >> ___
>> >> mesa-dev mailing list
>> >> mesa-dev@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> > ___
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa 2/6] nouveau: silence paranoid compiler's -Wclass-memaccess

2018-09-22 Thread Eric Engestrom 
On Saturday, 2018-09-22 10:27:50 -0400, Jan Vesely wrote:
> The warning is correct. In the first case, memset tries to zero "Target"
> object which has a non-trivial constructor and non-trivial
> copy-constructor. The original code is broken in the way it mixes C and C++
> initialization and the patch only papers over the issue.
> The correct fix would be to provide a proper constructor for structs that
> include instances of C++ classes.

Thanks for the analysis here, I'll drop these memaccess patches.
The correct fix is beyond what I'm willing to do though, so I'll let
someone else fix them :)

> 
> Jan
> 
> On Sat, Sep 22, 2018 at 6:43 AM Karol Herbst  wrote:
> 
> > yeah, I agree here. Either the code was wrong in the first place,
> > which means it would have to be fixed properly or the warning is
> > wrong. The proper fix here is that GCC should detect itself if it's
> > safe to do or not, otherwise that warning becomes a "might be a
> > problem" thing which doesn't help at all. Either it is wrong, or it
> > isn't. And gcc should be able to know in this case.
> >
> > On Sat, Sep 22, 2018 at 6:07 AM, Ilia Mirkin  wrote:
> > > Based on the various fixes, warning seems bogus -- is the proper
> > > solution -Wno-class-memaccess? (Or however one disables such
> > > things...)
> > >
> > > On Fri, Sep 21, 2018 at 9:50 AM, Eric Engestrom
> > >  wrote:
> > >> Signed-off-by: Eric Engestrom 
> > >> ---
> > >>  src/gallium/drivers/nouveau/codegen/nv50_ir.cpp| 2 +-
> > >>  src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp | 2 +-
> > >>  2 files changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> > b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> > >> index 49425b98b9137058c986..62ebc2d24069b7b5f523 100644
> > >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> > >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> > >> @@ -905,7 +905,7 @@ Instruction::isCommutationLegal(const Instruction
> > *i) const
> > >>  TexInstruction::TexInstruction(Function *fn, operation op)
> > >> : Instruction(fn, op, TYPE_F32)
> > >>  {
> > >> -   memset(, 0, sizeof(tex));
> > >> +   memset(static_cast(), 0, sizeof(tex));
> > >>
> > >> tex.rIndirectSrc = -1;
> > >> tex.sIndirectSrc = -1;
> > >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> > b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> > >> index 9193a01f189874a7fb38..b6b9b42964bec670079c 100644
> > >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> > >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> > >> @@ -454,7 +454,7 @@ CodeEmitter::addInterp(int ipa, int reg, FixupApply
> > apply)
> > >>if (!fixupInfo)
> > >>   return false;
> > >>if (n == 0)
> > >> - memset(fixupInfo, 0, sizeof(FixupInfo));
> > >> + memset(static_cast(fixupInfo), 0, sizeof(FixupInfo));
> > >> }
> > >> ++fixupInfo->count;
> > >>
> > >> --
> > >> Cheers,
> > >>   Eric
> > >>
> > >> ___
> > >> mesa-dev mailing list
> > >> mesa-dev@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > ___
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa 2/6] nouveau: silence paranoid compiler's -Wclass-memaccess

2018-09-22 Thread Jan Vesely 
The warning is correct. In the first case, memset tries to zero "Target"
object which has a non-trivial constructor and non-trivial
copy-constructor. The original code is broken in the way it mixes C and C++
initialization and the patch only papers over the issue.
The correct fix would be to provide a proper constructor for structs that
include instances of C++ classes.

Jan

On Sat, Sep 22, 2018 at 6:43 AM Karol Herbst  wrote:

> yeah, I agree here. Either the code was wrong in the first place,
> which means it would have to be fixed properly or the warning is
> wrong. The proper fix here is that GCC should detect itself if it's
> safe to do or not, otherwise that warning becomes a "might be a
> problem" thing which doesn't help at all. Either it is wrong, or it
> isn't. And gcc should be able to know in this case.
>
> On Sat, Sep 22, 2018 at 6:07 AM, Ilia Mirkin  wrote:
> > Based on the various fixes, warning seems bogus -- is the proper
> > solution -Wno-class-memaccess? (Or however one disables such
> > things...)
> >
> > On Fri, Sep 21, 2018 at 9:50 AM, Eric Engestrom
> >  wrote:
> >> Signed-off-by: Eric Engestrom 
> >> ---
> >>  src/gallium/drivers/nouveau/codegen/nv50_ir.cpp| 2 +-
> >>  src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> >> index 49425b98b9137058c986..62ebc2d24069b7b5f523 100644
> >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> >> @@ -905,7 +905,7 @@ Instruction::isCommutationLegal(const Instruction
> *i) const
> >>  TexInstruction::TexInstruction(Function *fn, operation op)
> >> : Instruction(fn, op, TYPE_F32)
> >>  {
> >> -   memset(, 0, sizeof(tex));
> >> +   memset(static_cast(), 0, sizeof(tex));
> >>
> >> tex.rIndirectSrc = -1;
> >> tex.sIndirectSrc = -1;
> >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> >> index 9193a01f189874a7fb38..b6b9b42964bec670079c 100644
> >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> >> @@ -454,7 +454,7 @@ CodeEmitter::addInterp(int ipa, int reg, FixupApply
> apply)
> >>if (!fixupInfo)
> >>   return false;
> >>if (n == 0)
> >> - memset(fixupInfo, 0, sizeof(FixupInfo));
> >> + memset(static_cast(fixupInfo), 0, sizeof(FixupInfo));
> >> }
> >> ++fixupInfo->count;
> >>
> >> --
> >> Cheers,
> >>   Eric
> >>
> >> ___
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa 2/6] nouveau: silence paranoid compiler's -Wclass-memaccess

2018-09-22 Thread Karol Herbst 
yeah, I agree here. Either the code was wrong in the first place,
which means it would have to be fixed properly or the warning is
wrong. The proper fix here is that GCC should detect itself if it's
safe to do or not, otherwise that warning becomes a "might be a
problem" thing which doesn't help at all. Either it is wrong, or it
isn't. And gcc should be able to know in this case.

On Sat, Sep 22, 2018 at 6:07 AM, Ilia Mirkin  wrote:
> Based on the various fixes, warning seems bogus -- is the proper
> solution -Wno-class-memaccess? (Or however one disables such
> things...)
>
> On Fri, Sep 21, 2018 at 9:50 AM, Eric Engestrom
>  wrote:
>> Signed-off-by: Eric Engestrom 
>> ---
>>  src/gallium/drivers/nouveau/codegen/nv50_ir.cpp| 2 +-
>>  src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp 
>> b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> index 49425b98b9137058c986..62ebc2d24069b7b5f523 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> @@ -905,7 +905,7 @@ Instruction::isCommutationLegal(const Instruction *i) 
>> const
>>  TexInstruction::TexInstruction(Function *fn, operation op)
>> : Instruction(fn, op, TYPE_F32)
>>  {
>> -   memset(, 0, sizeof(tex));
>> +   memset(static_cast(), 0, sizeof(tex));
>>
>> tex.rIndirectSrc = -1;
>> tex.sIndirectSrc = -1;
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp 
>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> index 9193a01f189874a7fb38..b6b9b42964bec670079c 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
>> @@ -454,7 +454,7 @@ CodeEmitter::addInterp(int ipa, int reg, FixupApply 
>> apply)
>>if (!fixupInfo)
>>   return false;
>>if (n == 0)
>> - memset(fixupInfo, 0, sizeof(FixupInfo));
>> + memset(static_cast(fixupInfo), 0, sizeof(FixupInfo));
>> }
>> ++fixupInfo->count;
>>
>> --
>> Cheers,
>>   Eric
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa 2/6] nouveau: silence paranoid compiler's -Wclass-memaccess

2018-09-21 Thread Ilia Mirkin 
Based on the various fixes, warning seems bogus -- is the proper
solution -Wno-class-memaccess? (Or however one disables such
things...)

On Fri, Sep 21, 2018 at 9:50 AM, Eric Engestrom
 wrote:
> Signed-off-by: Eric Engestrom 
> ---
>  src/gallium/drivers/nouveau/codegen/nv50_ir.cpp| 2 +-
>  src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp 
> b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> index 49425b98b9137058c986..62ebc2d24069b7b5f523 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
> @@ -905,7 +905,7 @@ Instruction::isCommutationLegal(const Instruction *i) 
> const
>  TexInstruction::TexInstruction(Function *fn, operation op)
> : Instruction(fn, op, TYPE_F32)
>  {
> -   memset(, 0, sizeof(tex));
> +   memset(static_cast(), 0, sizeof(tex));
>
> tex.rIndirectSrc = -1;
> tex.sIndirectSrc = -1;
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp 
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> index 9193a01f189874a7fb38..b6b9b42964bec670079c 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
> @@ -454,7 +454,7 @@ CodeEmitter::addInterp(int ipa, int reg, FixupApply apply)
>if (!fixupInfo)
>   return false;
>if (n == 0)
> - memset(fixupInfo, 0, sizeof(FixupInfo));
> + memset(static_cast(fixupInfo), 0, sizeof(FixupInfo));
> }
> ++fixupInfo->count;
>
> --
> Cheers,
>   Eric
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH mesa 2/6] nouveau: silence paranoid compiler's -Wclass-memaccess

2018-09-21 Thread Eric Engestrom 
Signed-off-by: Eric Engestrom 
---
 src/gallium/drivers/nouveau/codegen/nv50_ir.cpp| 2 +-
 src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp 
b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
index 49425b98b9137058c986..62ebc2d24069b7b5f523 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
@@ -905,7 +905,7 @@ Instruction::isCommutationLegal(const Instruction *i) const
 TexInstruction::TexInstruction(Function *fn, operation op)
: Instruction(fn, op, TYPE_F32)
 {
-   memset(, 0, sizeof(tex));
+   memset(static_cast(), 0, sizeof(tex));
 
tex.rIndirectSrc = -1;
tex.sIndirectSrc = -1;
diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp 
b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
index 9193a01f189874a7fb38..b6b9b42964bec670079c 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target.cpp
@@ -454,7 +454,7 @@ CodeEmitter::addInterp(int ipa, int reg, FixupApply apply)
   if (!fixupInfo)
  return false;
   if (n == 0)
- memset(fixupInfo, 0, sizeof(FixupInfo));
+ memset(static_cast(fixupInfo), 0, sizeof(FixupInfo));
}
++fixupInfo->count;
 
-- 
Cheers,
  Eric

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev