[Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Jan 18 05:25:54 UTC 2021


On Sat, Jan 16, 2021 at 3:57 PM Aditya <yashsri421 at gmail.com> wrote:
>
> On 16/1/21 7:08 pm, Dwaipayan Ray wrote:
> > On Sat, Jan 16, 2021 at 6:48 PM Aditya <yashsri421 at gmail.com> wrote:
> >>
> >> On 16/1/21 6:13 pm, Dwaipayan Ray wrote:
> >>> On Sat, Jan 16, 2021 at 5:52 PM Aditya Srivastava <yashsri421 at gmail.com> wrote:
> >>>>
> >>>> Local symbols prefixed with '.L' do not emit symbol table entries, as
> >>>> they have special meaning for the assembler.
> >>>>
> >>>> '.L' prefixed symbols can be used within a code region, but should be
> >>>> avoided for denoting a range of code via 'SYM_*_START/END' annotations.
> >>>>
> >>>> Add a new check to emit warning on finding the usage of '.L' symbols
> >>>> in '.S' files.
> >>>>
> >>>> Suggested-by: Mark Brown <broonie at kernel.org>
> >>>> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
> >>>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> >>>> ---
> >>>>  scripts/checkpatch.pl | 6 ++++++
> >>>>  1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>>> index 7030c4d6d126..87d96a039e64 100755
> >>>> --- a/scripts/checkpatch.pl
> >>>> +++ b/scripts/checkpatch.pl
> >>>> @@ -3590,6 +3590,12 @@ sub process {
> >>>>                         }
> >>>>                 }
> >>>>
> >>>> +# check for .L prefix local symbols in .S files
> >>>> +               if ($realfile =~ /\.S$/ && $line =~ /\.L\S+/) {
> >>>> +                       WARN("AVOID_L_PREFIX",
> >>>> +                            "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
> >>>> +               }
> >>>> +
> >>>>  # check we are in a valid source file C or perl if not then ignore this hunk
> >>>>                 next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
> >>>>
> >>>> --
> >>>> 2.17.1
> >>>
> >>> From an earlier conversation:
> >>>>   So basically, you can use an .L symbol *inside* a function or a code
> >>>>   segment, you just can't use the .L symbol to contain the code using a
> >>>>   SYM_*_START/END annotation pair.
> >>>
> >>> So this check warns on all uses of the .L prefix
> >>> I think that might be incorrect.
> >>>
> >>
> >> Hey Dwaipayan,
> >> I think you missed this:
> >>
> >>> - If the line contains ".L" prefixed symbol, give user a
> >>> warning/check, so that they can ensure that the line is not inside
> >>> START/END block. (As we may not be able to make sure about the same,
> >>> if the START/END line is not in the patch; otherwise we could run a
> >>> while loop)
> >>
> >
> > Then again what percentage of users actually intended to use
> > it in a local scope? That is actually a correct usage and checkpatch will
> > keep complaining - there will be no way to remove the warning.
> >
> >> At best, I think, we could use $context_function, which should work
> >> for patches, but again, this will not work for files.
> >>
> >> Do you have any suggestions?
> >>
> >
> > Does $context_function work for .S files? Am not sure.> But again that will not help much.
> >
> > The main concern is not to allow .L prefix within a
> > SYM_*_START...SYM_*_END annotation pair.
> >
> > That means something like this will be disallowed:
> >
> > SYM_FUNC_START(memset)
> >     .L ... etc
> > SYM_FUNC_END(memset)
> >
>
> Yes, $context_function works inside .S files.
> So, I think if it is inside one of the SYM_*_START contexts, it should
> work for us.
>
> > One possible way can be to extract the context after getting
> > a SYM_*_START. Following which the lines could be checked
> > for a .L preifx.
> > > But complexity wise I dont know if it would be worth it.
> >
>
> I think, we could do this for files.
>

Just run it on the current kernel source and find out how many cases
would generally match.

If it is just a few hundred occurrences, use a simple rule, not a
complex one that hardly works on patches.

Lukas


More information about the Linux-kernel-mentees mailing list