Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1593541 > unrolled thread

Re: [PATCH] objtool: drop redundant flags generation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] objtool: drop redundant flags generation Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-06 18:30 +0100
    Re: [PATCH] objtool: drop redundant flags generation Nicholas Mc Guire <der.herr@hofr.at> - 2017-03-06 19:50 +0100
      Re: [PATCH] objtool: drop redundant flags generation Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-06 22:50 +0100
        Re: [PATCH] objtool: drop redundant flags generation Nicholas Mc Guire <der.herr@hofr.at> - 2017-03-07 09:30 +0100
          Re: [PATCH] objtool: drop redundant flags generation Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-07 15:00 +0100
            Re: [PATCH] objtool: drop redundant flags generation Nicholas Mc Guire <der.herr@hofr.at> - 2017-03-07 15:50 +0100

#1593541 — Re: [PATCH] objtool: drop redundant flags generation

On Mon, Mar 06, 2017 at 06:00:25PM +0100, Nicholas Mc Guire wrote:
> The generator was emitting quite a few duplicate flags which was making
> doublebitand.cocci nervous. This awk hack resolves the duplicate issue.
> 
> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> ---
> 
> The coccinelle complaints emitted was about 230 findings total:
> ./arch/x86/lib/inat-tables.c:214:10-20: duplicated argument to & or |
> ./arch/x86/lib/inat-tables.c:214:23-33: duplicated argument to & or |
> ./arch/x86/lib/inat-tables.c:218:10-20: duplicated argument to & or |
> ./arch/x86/lib/inat-tables.c:218:23-33: duplicated argument to & or |
> ....
> ./tools/objtool/arch/x86/insn/inat-tables.c:214:10-20: duplicated argument to & or |
> ./tools/objtool/arch/x86/insn/inat-tables.c:214:23-33: duplicated argument to & or |
> ./tools/objtool/arch/x86/insn/inat-tables.c:218:10-20: duplicated argument to & or |
> ./tools/objtool/arch/x86/insn/inat-tables.c:218:23-33: duplicated argument to & or |
> ...
> spatch --sp-file scripts/coccinelle/tests/doublebitand.cocci inat-tables.c -D report
> will give you the full list - all are caused by duplicates in the generated
> output by the add_flags function in the two instances of gen-insn-attr-x86.awk.
> 
> Q: The two copies of gen-insn-attr-x86.awk are identical and its not actually clear
>    why this duplication is needed ? Further the maintainers list emitted for the
>    two files differ.
> 
> Patch was checked by manual review of the diff between the initial file and the
> regenerated file after the below patch was applied.
> Second verification was by make tools/objtool and comparing the generated binaries
> in tools/objtool/arch/x86/decode.o with diff.
> 
> Patch is against 4.11-rc1 (localversion-next is next-20170306)
> 
>  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.

-- 
Josh

[toc] | [next] | [standalone]

#1593585

On Mon, Mar 06, 2017 at 11:25:37AM -0600, Josh Poimboeuf wrote:
> On Mon, Mar 06, 2017 at 06:00:25PM +0100, Nicholas Mc Guire wrote:
> > The generator was emitting quite a few duplicate flags which was making
> > doublebitand.cocci nervous. This awk hack resolves the duplicate issue.
> > 
> > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> > ---
> > 
> > The coccinelle complaints emitted was about 230 findings total:
> > ./arch/x86/lib/inat-tables.c:214:10-20: duplicated argument to & or |
> > ./arch/x86/lib/inat-tables.c:214:23-33: duplicated argument to & or |
> > ./arch/x86/lib/inat-tables.c:218:10-20: duplicated argument to & or |
> > ./arch/x86/lib/inat-tables.c:218:23-33: duplicated argument to & or |
> > ....
> > ./tools/objtool/arch/x86/insn/inat-tables.c:214:10-20: duplicated argument to & or |
> > ./tools/objtool/arch/x86/insn/inat-tables.c:214:23-33: duplicated argument to & or |
> > ./tools/objtool/arch/x86/insn/inat-tables.c:218:10-20: duplicated argument to & or |
> > ./tools/objtool/arch/x86/insn/inat-tables.c:218:23-33: duplicated argument to & or |
> > ...
> > spatch --sp-file scripts/coccinelle/tests/doublebitand.cocci inat-tables.c -D report
> > will give you the full list - all are caused by duplicates in the generated
> > output by the add_flags function in the two instances of gen-insn-attr-x86.awk.
> > 
> > Q: The two copies of gen-insn-attr-x86.awk are identical and its not actually clear
> >    why this duplication is needed ? Further the maintainers list emitted for the
> >    two files differ.
> > 
> > Patch was checked by manual review of the diff between the initial file and the
> > regenerated file after the below patch was applied.
> > Second verification was by make tools/objtool and comparing the generated binaries
> > in tools/objtool/arch/x86/decode.o with diff.
> > 
> > Patch is against 4.11-rc1 (localversion-next is next-20170306)
> > 
> >  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 ?

thx!
hofrat

[toc] | [prev] | [next] | [standalone]

#1593756

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.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1593974

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.

So the question is does it make sense to fix up this one aspect
or is this "fixing" an intermediate mess only that will go away,
once consolidation happens, anyway.

thx!
hofrat

[toc] | [prev] | [next] | [standalone]

#1594247

On Tue, 7 Mar 2017 08:13:19 +0000
Nicholas Mc Guire <der.herr@hofr.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?
I recommend you to change coccicheck to avoid checking
auto-generated code first.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1594312

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@hofr.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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web