Message-ID: <20170307141847.GA1559@osadl.at>
Date:   Tue, 7 Mar 2017 14:18:48 +0000
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Adrian Hunter <adrian.hunter@...el.com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] objtool: drop redundant flags generation

On Tue, Mar 07, 2017 at 02:55:21PM +0100, Masami Hiramatsu wrote:
> On Tue, 7 Mar 2017 08:13:19 +0000
> Nicholas Mc Guire <der.herr@...r.at> wrote:
> 
> > On Mon, Mar 06, 2017 at 03:40:54PM -0600, Josh Poimboeuf wrote:
> > > On Mon, Mar 06, 2017 at 05:54:01PM +0000, Nicholas Mc Guire wrote:
> > > > On Mon, Mar 06, 2017 at 11:25:37AM -0600, Josh Poimboeuf wrote:
> > > > > >  arch/x86/tools/gen-insn-attr-x86.awk              | 12 ++++++++++--
> > > > > >  tools/objtool/arch/x86/insn/gen-insn-attr-x86.awk | 12 ++++++++++--
> > > > > 
> > > > > There's actually a third copy of the decoder in:
> > > > > 
> > > > >   tools/perf/util/intel-pt-decoder/
> > > > > 
> > > > > Yes, the duplication is a pain, but it's part of an effort to keep
> > > > > 'tools/*' source independent from kernel code.
> > > > > 
> > > > > Maybe we can at least combine the objtool and perf versions someday.
> > > > >
> > > > Bad - missed that one - did not build perf - the generator seems to
> > > > be the same though only differing by a single blank line - so pulling 
> > > > those together should be a non-issue atleast with respect to the 
> > > > generator as the x86-opcode-map.txt are all the same ? ...or what 
> > > > fun am I missing ?
> > > 
> > > In theory, all three copies of the decoder should be identical.  That
> > > includes all the files: insn.[ch], inat.[ch], inat_types.h,
> > > gen-insn-attr-x86.awk, x86-opcode-map.txt.
> > >
> > Understood - but this is a different problem that is being
> > addressed with this cleanup - the duplicates make no sense in
> > any case as far as I can see - with or without consolidation of
> > the other files (and x86-opcode-map.txt does seem to be the
> > same in all 3 cases) - the point was that it is causing a quite
> > large number of coccicheck warnings which are iritating and
> > in this case easy to remove.
> 
> Why would you apply coccinelle to auto-generated code?

Why should autogenerated code have the priviledge of non-conformance
and limited understandability ?
Coccicheck is applied to the entire kernel and it is not imediately 
visible/clear what is autogenerated and what not and I see no reason
why autogenerated code should not conform to coding standards or 
to readability rules. Now in this particular case it might well be
thta nobody ever reads that generated file anyway - I cant say - never
the less it will show up in test-harneses as noise.

> I recommend you to change coccicheck to avoid checking
> auto-generated code first.

Frist there is no canonic way to even detect if code is generated or not 
and further if you give me a good reason why generated code does not need 
to follow coding standards and does not need to be readable and/or 
understandable ?

We have autogenerated code in the kernel that is so unbelievably
braindead it is scary that it ever made it into mainline !
Here a few highlights that coccinelle found with some scripts
that are not in mainline:

drivers/media/dvb-frontends/dib7000m.c:926 
  /* P_dintl_native, P_dintlv_inv, 
     P_hrch, P_code_rate, P_select_hp */
  value = 0; 
  if (1 != 0) 
          value |= (1 << 6); 
  if (ch->hierarchy == 1) 
          value |= (1 << 4);
  if (1 == 1)
          value |= 1;
  switch ((ch->hierarchy == 0 || 1 == 1) ?
          ch->code_rate_HP : ch->code_rate_LP) {

aside from braindead control flow - the comment has nothing to do with the
code - explaination by author: generated code !

drivers/staging/rtl8723au/hal/rtl8723a\_bt-coexist.c:7264 else duplicates if
   ...
   } else if (maxInterval == 2) {
           btdm_2AntPsTdma(padapter, true, 15);
           pBtdm8723->psTdmaDuAdjType = 15;
   } else if (maxInterval == 3) {
           btdm_2AntPsTdma(padapter, true, 15);
           pBtdm8723->psTdmaDuAdjType = 15;
   } else {
           btdm_2AntPsTdma(padapter, true, 15);
           pBtdm8723->psTdmaDuAdjType = 15;
   }

also an example of generated code - CamelCase naming +
totally usless control flow (actually its 56 lines that
collaps into a single if/else) - if you ever hit a problem
in such code it probably qualifies as hedonistic outbreak.

Now the generated code in the case of gen-insn-attr-x86.awk 
is admitedly not in the same "class" as the above examples but
never the less there is no reason for it, and generated code
in general, to reduce readable, understandable and maintainable 
notably long term (someone may need to look at autogenerated 
code in 15 years with the specific tools no longer avaialble).

In this specific case - given the overall complexity/size of
gen-insn-attr-x86.awk I do not quite see the big issue with adding
those 10 lines to remove almost 400 coccinelle warnings - but
I´m also not into the specifics of that code so if you feel that
it would cause more problems than benefit no issue with not taking
the proposed patch (and if someone has a nicer awk solution that
might be even better !)

thx!
hofrat

Powered by blists - more mailing lists