Re: [PATCH] Enable '-Werror' by default for all kernel builds

From: Harry Wentland <harry.wentland@amd.com>
To: "Linus Torvalds" <torvalds@linux-foundation.org>,
	"Arnd Bergmann" <arnd@kernel.org>, "Leo Li" <sunpeng.li@amd.com>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Christian König" <christian.koenig@amd.com>,
	"Pan, Xinhui" <Xinhui.Pan@amd.com>
Cc: Nathan Chancellor <nathan@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	llvm@lists.linux.dev, Nick Desaulniers <ndesaulniers@google.com>,
	amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] Enable '-Werror' by default for all kernel builds
Date: Tue, 7 Sep 2021 17:07:31 -0400	[thread overview]
Message-ID: <fb50dc44-fbe4-e6b3-5abc-d90375992292@amd.com> (raw)
In-Reply-To: <CAHk-=wh37UWTqUzbh5qg_x9pFgqBVwpdq_Kf+hnB5mqEUkrjmg@mail.gmail.com>

On 2021-09-07 1:33 p.m., Linus Torvalds wrote:
> On Tue, Sep 7, 2021 at 10:10 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Do I know why? No. I do note that that code is disgusting.
>>
>> It's passing one of those structs around by value, for example. That's
>> a 72-byte structure that is copied on the stack due to stupid calling
>> conventions. Maybe clang generates a few extra temporaries for it as
>> part of the function call stack setup? Who knows..
> 
> Ooh, yes.
> 
> This attached patch is crap - it converts the helper functions to use
> const pointers instead of passing the whole structure, but it then
> only converts that one file that *uses* them.
> 
> So the end result will not compile in general, but you can do
> 
>     make drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn30/display_rq_dlg_calc_30.o
> 
> and it compiles for me.
> 
> And while gcc doesn't care that much - it will apparently either
> generate the argument stack every call - clang cares deeply.
> 
> The nasty 720-byte stack frame that clang generates turns into just a
> 320-byte one, and code generation in general looks a *lot* better.
> 
> Now, as mentioned, this patch is broken and incomplete. But I really
> think the AMD GPU people need to do this. It makes those functions go
> from practically unusable to not horribly disgusting.
> 
> So Harry/Leo/Alex/Christian and amd-gfx list - can you look into
> making this ugly "make one file compile better" patch actually work
> properly?
> 

Yes, will take a look at this tonight. We definitely shouldn't be passing
large structs by value.

Harry

> It *looks* lto me ike that code was perhaps written for a C++ compiler
> and the helpers have been written as a "pass by reference", and the
> arguments used to be
> 
>      const display_data_rq_misc_params_st& rq_misc_param
> 
> and then the compiler will pass the argument as a pointer. And then it
> was converted to C, and the "pass by reference" in the function
> declaration was turned into "pass by value", to avoid changing "." to
> "->" in the use.
> 
> But a '&arg' thing in C++ really is a '*arg' pointer in C, and should
> have been done as that.
> 
> Of course, it's also possible that that code was simply written by
> somebody who didn't understand just *how* horrible it is to pass
> structures bigger than a word or two by value.
> 
> Do we have a compiler warning for passing big structures by value?
> 
>                    Linus
> 