Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932680AbdCGOpC (ORCPT ); Tue, 7 Mar 2017 09:45:02 -0500 Received: from 92-243-34-74.adsl.nanet.at ([92.243.34.74]:43642 "EHLO mail.osadl.at" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1755563AbdCGOnt (ORCPT ); Tue, 7 Mar 2017 09:43:49 -0500 Date: Tue, 7 Mar 2017 14:18:48 +0000 From: Nicholas Mc Guire To: Masami Hiramatsu Cc: Josh Poimboeuf , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Adrian Hunter , Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org Subject: Re: [PATCH] objtool: drop redundant flags generation Message-ID: <20170307141847.GA1559@osadl.at> References: <1488819625-27395-1-git-send-email-der.herr@hofr.at> <20170306172537.uox2f7ru3x72jzea@treble> <20170306175401.GA26026@osadl.at> <20170306214054.tobodci764xvbwr7@treble> <20170307081319.GA31027@osadl.at> <20170307145521.887324964e1e468822dba0a3@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170307145521.887324964e1e468822dba0a3@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4916 Lines: 112 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 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