Build fail in drivers/gpu/drm/i915/display/intel_tc.c

Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[David Laight]

I'm seeing a build fail at line 1878 of intel_tc.c

gcc is being a bit over-enthusiastic about snprintf() formats
and reports that "%c/TC#%d", xxx, tc_port + 1 might not fit
in the 8 bytes available.
'tc_port' is an enum with values -1 to 5.
I guess it is either allowing for the full 'int' value of
the enum.
Changing to '(tc_port & 7) + 1' stops the compiler bleating.

The code isn't new, so I guess the compiler flags have changed?

It might depend on the gcc version as well.
I'm using gcc 7.5.0 from Ubuntu 18.04.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[Imre Deak]

On Thu, Nov 09, 2023 at 10:55:38PM +0000, David Laight wrote:
> I'm seeing a build fail at line 1878 of intel_tc.c
> 
> gcc is being a bit over-enthusiastic about snprintf() formats
> and reports that "%c/TC#%d", xxx, tc_port + 1 might not fit
> in the 8 bytes available.
> 'tc_port' is an enum with values -1 to 5.
> I guess it is either allowing for the full 'int' value of
> the enum.
> Changing to '(tc_port & 7) + 1' stops the compiler bleating.
> 
> The code isn't new, so I guess the compiler flags have changed?
> 
> It might depend on the gcc version as well.
> I'm using gcc 7.5.0 from Ubuntu 18.04.

The compiler warn should be fixed/suppressed by:
https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@intel.com

--Imre

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[Linus Torvalds]

On Thu, 9 Nov 2023 at 15:34, Imre Deak <imre.deak@intel.com> wrote:
>
> The compiler warn should be fixed/suppressed by:
> https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@intel.com

Ugh, so now it's a dynamic allocation, wasting memory, and a pointer
to it, using as much memory as the array did in the first place.

All because of a pointless warning that was a false positive - and was
always harmless anyway, since snprintf() is safe (ie it was only a
"might be truncated").

Please don't do this. Either do that ((tc_port & 7) + 1) suggestion of
David's, or just do '%c' and make the expression be

  '1' + tc_port

which should be fine since I915_MAX_PORTS is 8 or whatever.

I do wonder why those ports are printed out as '1-8', when the 'enum
port' is PORT_A..I.

So it would actually have made more sense to print them out as %c with
the expression being

   'A'+tc_port

but I guess by now people might depend on the 1..8 naming?

             Linus

RE: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[David Laight]

From: Linus Torvalds
> Sent: 10 November 2023 00:52
> 
> On Thu, 9 Nov 2023 at 15:34, Imre Deak <imre.deak@intel.com> wrote:
> >
> > The compiler warn should be fixed/suppressed by:
> > https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@intel.com
> 
> Ugh, so now it's a dynamic allocation, wasting memory, and a pointer
> to it, using as much memory as the array did in the first place.
> 
> All because of a pointless warning that was a false positive - and was
> always harmless anyway, since snprintf() is safe (ie it was only a
> "might be truncated").

That entire warning for snprintf() is a false positive.
The ones that are likely to overflow unexpectedly are the ones
with a "%s" format for a 'char *' pointer where there is no
implied length.

The same check for printf() using the implied buffer length
probably does make sense.

I don't even think there is a way of avoiding it on a case by case
basis - apart from passing both the buffer address and length
to an inline asm that the compiler has to assume might change
the values, but that tends to generate an extra 'mov' instruction
for no good reason at all.

> 
> Please don't do this. Either do that ((tc_port & 7) + 1) suggestion of
> David's, or just do '%c' and make the expression be
> 
>   '1' + tc_port
> 
> which should be fine since I915_MAX_PORTS is 8 or whatever.

If I'd though for 2 seconds that is what I'd have done.
But I wanted to get something through the compiler.

> I do wonder why those ports are printed out as '1-8', when the 'enum
> port' is PORT_A..I.

They look like TC_PORT_[1..6] to me - the enum values are 0..5
which is why there is a 'random' '+ 1'.

	David

> 
> So it would actually have made more sense to print them out as %c with
> the expression being
> 
>    'A'+tc_port
> 
> but I guess by now people might depend on the 1..8 naming?
> 
>              Linus

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[Imre Deak]

On Fri, Nov 10, 2023 at 09:00:21AM +0000, David Laight wrote:
> From: Linus Torvalds
> > Sent: 10 November 2023 00:52
> > 
> > On Thu, 9 Nov 2023 at 15:34, Imre Deak <imre.deak@intel.com> wrote:
> > >
> > > The compiler warn should be fixed/suppressed by:
> > > https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@intel.com
> > 
> > Ugh, so now it's a dynamic allocation, wasting memory, and a pointer
> > to it, using as much memory as the array did in the first place.
> > 
> > All because of a pointless warning that was a false positive - and was
> > always harmless anyway, since snprintf() is safe (ie it was only a
> > "might be truncated").
> 
> That entire warning for snprintf() is a false positive.
> The ones that are likely to overflow unexpectedly are the ones
> with a "%s" format for a 'char *' pointer where there is no
> implied length.
> 
> The same check for printf() using the implied buffer length
> probably does make sense.
> 
> I don't even think there is a way of avoiding it on a case by case
> basis - apart from passing both the buffer address and length
> to an inline asm that the compiler has to assume might change
> the values, but that tends to generate an extra 'mov' instruction
> for no good reason at all.
> 
> > 
> > Please don't do this. Either do that ((tc_port & 7) + 1) suggestion of
> > David's, or just do '%c' and make the expression be
> > 
> >   '1' + tc_port
> > 
> > which should be fine since I915_MAX_PORTS is 8 or whatever.

Ok, the patch above was merged already to drm-tip, but I agree not to
use kasprintf for this. The above looks ok and there is already
tc_port_name() for this, would just need to export that from
intel_display.h.

I can follow up with a patch for this, or if you or David wanted to do
that please send a patch to the intel-gfx@lists.freedesktop.org list.

Thanks,
Imre

> If I'd though for 2 seconds that is what I'd have done.
> But I wanted to get something through the compiler.
> 
> > I do wonder why those ports are printed out as '1-8', when the 'enum
> > port' is PORT_A..I.
> 
> They look like TC_PORT_[1..6] to me - the enum values are 0..5
> which is why there is a 'random' '+ 1'.
> 
> 	David
> 
> > 
> > So it would actually have made more sense to print them out as %c with
> > the expression being
> > 
> >    'A'+tc_port
> > 
> > but I guess by now people might depend on the 1..8 naming?
> > 
> >              Linus
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

RE: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[David Laight]

From: Imre Deak
> Sent: 14 November 2023 19:08
> 
> On Fri, Nov 10, 2023 at 09:00:21AM +0000, David Laight wrote:
> > From: Linus Torvalds
> > > Sent: 10 November 2023 00:52
> > >
> > > On Thu, 9 Nov 2023 at 15:34, Imre Deak <imre.deak@intel.com> wrote:
> > > >
> > > > The compiler warn should be fixed/suppressed by:
> > > > https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@intel.com
> > >
> > > Ugh, so now it's a dynamic allocation, wasting memory, and a pointer
> > > to it, using as much memory as the array did in the first place.
> > >
> > > All because of a pointless warning that was a false positive - and was
> > > always harmless anyway, since snprintf() is safe (ie it was only a
> > > "might be truncated").
> >
> > That entire warning for snprintf() is a false positive.
> > The ones that are likely to overflow unexpectedly are the ones
> > with a "%s" format for a 'char *' pointer where there is no
> > implied length.
> >
> > The same check for printf() using the implied buffer length
> > probably does make sense.
> >
> > I don't even think there is a way of avoiding it on a case by case
> > basis - apart from passing both the buffer address and length
> > to an inline asm that the compiler has to assume might change
> > the values, but that tends to generate an extra 'mov' instruction
> > for no good reason at all.
> >
> > >
> > > Please don't do this. Either do that ((tc_port & 7) + 1) suggestion of
> > > David's, or just do '%c' and make the expression be
> > >
> > >   '1' + tc_port
> > >
> > > which should be fine since I915_MAX_PORTS is 8 or whatever.
> 
> Ok, the patch above was merged already to drm-tip,

I'd noticed ....

> but I agree not to use kasprintf for this.
> The above looks ok and there is already
> tc_port_name() for this, would just need to export that from
> intel_display.h.

I'm not sure that #define does anything except obscure what
is going on.
Anyone reading the code is going to search for it and then sigh.
But it is your code :-)

> I can follow up with a patch for this, or if you or David wanted to do
> that please send a patch to the intel-gfx@lists.freedesktop.org list.

Probably much easier for you to do it.
My process to get patches emailed is a PITA.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)