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

[Aditya]

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.

But I am not sure about the complexity part.
Lukas, what do you think?

Thanks
Aditya