* [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
@ 2021-01-16 12:22 Aditya Srivastava
2021-01-16 12:43 ` Dwaipayan Ray
0 siblings, 1 reply; 21+ messages in thread
From: Aditya Srivastava @ 2021-01-16 12:22 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees, yashsri421
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@kernel.org>
Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
Signed-off-by: Aditya Srivastava <yashsri421@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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-16 12:22 [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files Aditya Srivastava
@ 2021-01-16 12:43 ` Dwaipayan Ray
2021-01-16 13:18 ` Aditya
0 siblings, 1 reply; 21+ messages in thread
From: Dwaipayan Ray @ 2021-01-16 12:43 UTC (permalink / raw)
To: Aditya Srivastava; +Cc: linux-kernel-mentees
On Sat, Jan 16, 2021 at 5:52 PM Aditya Srivastava <yashsri421@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@kernel.org>
> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
> Signed-off-by: Aditya Srivastava <yashsri421@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.
Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-16 12:43 ` Dwaipayan Ray
@ 2021-01-16 13:18 ` Aditya
2021-01-16 13:38 ` Dwaipayan Ray
0 siblings, 1 reply; 21+ messages in thread
From: Aditya @ 2021-01-16 13:18 UTC (permalink / raw)
To: Dwaipayan Ray; +Cc: linux-kernel-mentees
On 16/1/21 6:13 pm, Dwaipayan Ray wrote:
> On Sat, Jan 16, 2021 at 5:52 PM Aditya Srivastava <yashsri421@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@kernel.org>
>> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
>> Signed-off-by: Aditya Srivastava <yashsri421@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)
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?
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-16 13:18 ` Aditya
@ 2021-01-16 13:38 ` Dwaipayan Ray
2021-01-16 14:57 ` Aditya
0 siblings, 1 reply; 21+ messages in thread
From: Dwaipayan Ray @ 2021-01-16 13:38 UTC (permalink / raw)
To: Aditya; +Cc: linux-kernel-mentees
On Sat, Jan 16, 2021 at 6:48 PM Aditya <yashsri421@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@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@kernel.org>
> >> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
> >> Signed-off-by: Aditya Srivastava <yashsri421@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)
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.
Thanks & Regards,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-16 13:38 ` Dwaipayan Ray
@ 2021-01-16 14:57 ` Aditya
2021-01-18 5:25 ` Lukas Bulwahn
0 siblings, 1 reply; 21+ messages in thread
From: Aditya @ 2021-01-16 14:57 UTC (permalink / raw)
To: Dwaipayan Ray; +Cc: linux-kernel-mentees
On 16/1/21 7:08 pm, Dwaipayan Ray wrote:
> On Sat, Jan 16, 2021 at 6:48 PM Aditya <yashsri421@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@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@kernel.org>
>>>> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
>>>> Signed-off-by: Aditya Srivastava <yashsri421@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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-16 14:57 ` Aditya
@ 2021-01-18 5:25 ` Lukas Bulwahn
2021-01-18 14:44 ` Aditya Srivastava
0 siblings, 1 reply; 21+ messages in thread
From: Lukas Bulwahn @ 2021-01-18 5:25 UTC (permalink / raw)
To: Aditya; +Cc: Dwaipayan Ray, linux-kernel-mentees
On Sat, Jan 16, 2021 at 3:57 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 16/1/21 7:08 pm, Dwaipayan Ray wrote:
> > On Sat, Jan 16, 2021 at 6:48 PM Aditya <yashsri421@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@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@kernel.org>
> >>>> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
> >>>> Signed-off-by: Aditya Srivastava <yashsri421@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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-18 5:25 ` Lukas Bulwahn
@ 2021-01-18 14:44 ` Aditya Srivastava
2021-01-18 17:24 ` Lukas Bulwahn
0 siblings, 1 reply; 21+ messages in thread
From: Aditya Srivastava @ 2021-01-18 14:44 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees, yashsri421
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, if it lies within SYM_*_START/END annotation pair.
Suggested-by: Mark Brown <bro...@kernel.org>
Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
scripts/checkpatch.pl | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7030c4d6d126..858b5def61e9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2501,6 +2501,9 @@ sub process {
my $checklicenseline = 1;
+ # record SYM_*_START/END annotation pair count, for AVOID_L_PREFIX
+ my $sym_start_block = 0;
+
sanitise_line_reset();
my $line;
foreach my $rawline (@rawlines) {
@@ -3590,6 +3593,25 @@ sub process {
}
}
+# check for .L prefix local symbols in .S files
+ if ($realfile =~ /\.S$/) {
+ if ($line =~ /SYM_.*_START/ ||
+ (defined $context_function && $context_function =~ /SYM_.*_START/)) {
+ $sym_start_block++;
+ }
+
+ if ($line=~ /\.L\S+/ && # line contains .L prefixed local symbol
+ $sym_start_block > 0) { # lies between SYM_*_START and SYM_*_END pair
+ 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);
+ }
+
+ if ($line =~ /SYM_.*_END/ ||
+ (defined $context_function && $context_function =~ /SYM_.*_END/)) {
+ $sym_start_block--;
+ }
+ }
+
# 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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-18 14:44 ` Aditya Srivastava
@ 2021-01-18 17:24 ` Lukas Bulwahn
2021-01-18 18:42 ` Aditya
0 siblings, 1 reply; 21+ messages in thread
From: Lukas Bulwahn @ 2021-01-18 17:24 UTC (permalink / raw)
To: Aditya Srivastava; +Cc: Dwaipayan Ray, linux-kernel-mentees
On Mon, Jan 18, 2021 at 3:44 PM Aditya Srivastava <yashsri421@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
to emit a warning
> in '.S' files, if it lies within SYM_*_START/END annotation pair.
>
> Suggested-by: Mark Brown <bro...@kernel.org>
Hmm, that is not an email?
Did you test running it on the patch that introduced the bug initially
which then led to the overall discussion I pointed out to you?
Lukas
> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> scripts/checkpatch.pl | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7030c4d6d126..858b5def61e9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2501,6 +2501,9 @@ sub process {
>
> my $checklicenseline = 1;
>
> + # record SYM_*_START/END annotation pair count, for AVOID_L_PREFIX
> + my $sym_start_block = 0;
> +
> sanitise_line_reset();
> my $line;
> foreach my $rawline (@rawlines) {
> @@ -3590,6 +3593,25 @@ sub process {
> }
> }
>
> +# check for .L prefix local symbols in .S files
> + if ($realfile =~ /\.S$/) {
> + if ($line =~ /SYM_.*_START/ ||
> + (defined $context_function && $context_function =~ /SYM_.*_START/)) {
> + $sym_start_block++;
> + }
> +
> + if ($line=~ /\.L\S+/ && # line contains .L prefixed local symbol
> + $sym_start_block > 0) { # lies between SYM_*_START and SYM_*_END pair
> + 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);
> + }
> +
> + if ($line =~ /SYM_.*_END/ ||
> + (defined $context_function && $context_function =~ /SYM_.*_END/)) {
> + $sym_start_block--;
> + }
> + }
> +
> # 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
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-18 17:24 ` Lukas Bulwahn
@ 2021-01-18 18:42 ` Aditya
2021-01-18 19:13 ` Aditya Srivastava
0 siblings, 1 reply; 21+ messages in thread
From: Aditya @ 2021-01-18 18:42 UTC (permalink / raw)
To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees
On 18/1/21 10:54 pm, Lukas Bulwahn wrote:
> On Mon, Jan 18, 2021 at 3:44 PM Aditya Srivastava <yashsri421@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
>
> to emit a warning
>
Okay
>> in '.S' files, if it lies within SYM_*_START/END annotation pair.
>>
>> Suggested-by: Mark Brown <bro...@kernel.org>
>
> Hmm, that is not an email?
>
Oops, I'll fix it.
> Did you test running it on the patch that introduced the bug initially
> which then led to the overall discussion I pointed out to you?
>
Hi Lukas
So, the patch led to the initial discussion was probably this:
https://groups.google.com/g/clang-built-linux/c/1C6YoJKBsQQ/m/a8IS1NjGAgAJ
This patch is related to changes made in tools/objtool. So, we
probably cannot test this patch for AVOID_L_PREFIX warning.
I'll walk you through the discussion:
Because of the above changes in objtool, [PATCH] x86/entry: use
STB_GLOBAL for register restoring thunk was proposed
(https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/qWnKkZWFBgAJ)
It was later decided that the changes can be instead made to .L prefix
local symbols here for avoiding the objtool warning:
https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/MtjkJK6LCgAJ
And for standardizing this practice, the suggestion for checkpatch
rule was made here:
https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/4staOlf-CgAJ
However, I have tested the patch on arch/x86/ files and also the patch
proposed here fixing for one of those files.
As expected, I got no warnings for the patch fixing the warnings for
thunk_64.S file, and some warnings for files where the prefix was used.
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-18 18:42 ` Aditya
@ 2021-01-18 19:13 ` Aditya Srivastava
2021-01-19 16:32 ` Lukas Bulwahn
0 siblings, 1 reply; 21+ messages in thread
From: Aditya Srivastava @ 2021-01-18 19:13 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees, yashsri421
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 a warning on finding the usage of '.L' symbols
in '.S' files, if it lies within SYM_*_START/END annotation pair.
Suggested-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
scripts/checkpatch.pl | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7030c4d6d126..858b5def61e9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2501,6 +2501,9 @@ sub process {
my $checklicenseline = 1;
+ # record SYM_*_START/END annotation pair count, for AVOID_L_PREFIX
+ my $sym_start_block = 0;
+
sanitise_line_reset();
my $line;
foreach my $rawline (@rawlines) {
@@ -3590,6 +3593,25 @@ sub process {
}
}
+# check for .L prefix local symbols in .S files
+ if ($realfile =~ /\.S$/) {
+ if ($line =~ /SYM_.*_START/ ||
+ (defined $context_function && $context_function =~ /SYM_.*_START/)) {
+ $sym_start_block++;
+ }
+
+ if ($line=~ /\.L\S+/ && # line contains .L prefixed local symbol
+ $sym_start_block > 0) { # lies between SYM_*_START and SYM_*_END pair
+ 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);
+ }
+
+ if ($line =~ /SYM_.*_END/ ||
+ (defined $context_function && $context_function =~ /SYM_.*_END/)) {
+ $sym_start_block--;
+ }
+ }
+
# 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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-18 19:13 ` Aditya Srivastava
@ 2021-01-19 16:32 ` Lukas Bulwahn
0 siblings, 0 replies; 21+ messages in thread
From: Lukas Bulwahn @ 2021-01-19 16:32 UTC (permalink / raw)
To: Aditya Srivastava; +Cc: Dwaipayan Ray, linux-kernel-mentees
On Mon, Jan 18, 2021 at 8:14 PM Aditya Srivastava <yashsri421@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 a warning on finding the usage of '.L' symbols
> in '.S' files, if it lies within SYM_*_START/END annotation pair.
>
> Suggested-by: Mark Brown <broonie@kernel.org>
> Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
Okay, send it out to the initial discussion group (with the standard
CCs for mentees and checkpatch) and lkml. Then we see.
Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-22 20:13 ` Aditya
@ 2021-01-22 20:36 ` Fāng-ruì Sòng via Linux-kernel-mentees
0 siblings, 0 replies; 21+ messages in thread
From: Fāng-ruì Sòng via Linux-kernel-mentees @ 2021-01-22 20:36 UTC (permalink / raw)
To: Aditya
Cc: dwaipayanray1, LKML, clang-built-linux, Mark Brown, Joe Perches,
linux-kernel-mentees
On Fri, Jan 22, 2021 at 12:13 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 23/1/21 12:40 am, Joe Perches wrote:
> > On Fri, 2021-01-22 at 18:48 +0530, Aditya wrote:
> >> On 21/1/21 12:13 am, Joe Perches wrote:
> >>> I believe the test should be:
> >>>
> >>> if ($realfile =~ /\.S$/ &&
> >>> $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
> >>
> >> Joe, with this regex, we won't be able to match
> >> "jmp .L_restore
> >> SYM_FUNC_END(\name)"
> >
> > I think that's not an issue.
> >
> >> which was replaced in this patch in the discussion:
> >> https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/phKe-Tb6CgAJ
> >>
> >> Here, "jmp .L_restore" was also replaced to fix the error.
> >
> > Notice that this line was also replaced in the same patch:
> >
> > #ifdef CONFIG_PREEMPTION
> > -SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
> > +SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
> >
> >
> >> However, if this
> >> regex(/^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) is
> >> correct, maybe we don't need to check for $file as we are now checking
> >> for just one line.
> >>
> >> What do you think?
> >
> > The test I wrote was complete, did not use $file and emits a
> > warning on the use of CODE_SYM_START_LOCAL_NOALIGN(.L_restore)
> >
> > I think it's sufficient.
> >
>
> Okay, Thanks.. I will send the modified patch :)
>
> Thanks
> Aditya
I am slightly concerned with the direction here. jmp .Lsym is fine
and is probably preferable in cases where you absolutely want to emit
a local symbol.
(there is a related -z unique-symbol GNU ld+FGKASLR thing I shall
analyze in my spare time)
If the problem is just that STT_SECTION symbols are missing, can
objtool do something to conceptually synthesize them?
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-22 19:10 ` Joe Perches
@ 2021-01-22 20:13 ` Aditya
2021-01-22 20:36 ` Fāng-ruì Sòng via Linux-kernel-mentees
0 siblings, 1 reply; 21+ messages in thread
From: Aditya @ 2021-01-22 20:13 UTC (permalink / raw)
To: Joe Perches, linux-kernel
Cc: broonie, linux-kernel-mentees, clang-built-linux, dwaipayanray1
On 23/1/21 12:40 am, Joe Perches wrote:
> On Fri, 2021-01-22 at 18:48 +0530, Aditya wrote:
>> On 21/1/21 12:13 am, Joe Perches wrote:
>>> I believe the test should be:
>>>
>>> if ($realfile =~ /\.S$/ &&
>>> $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
>>
>> Joe, with this regex, we won't be able to match
>> "jmp .L_restore
>> SYM_FUNC_END(\name)"
>
> I think that's not an issue.
>
>> which was replaced in this patch in the discussion:
>> https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/phKe-Tb6CgAJ
>>
>> Here, "jmp .L_restore" was also replaced to fix the error.
>
> Notice that this line was also replaced in the same patch:
>
> #ifdef CONFIG_PREEMPTION
> -SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
> +SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
>
>
>> However, if this
>> regex(/^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) is
>> correct, maybe we don't need to check for $file as we are now checking
>> for just one line.
>>
>> What do you think?
>
> The test I wrote was complete, did not use $file and emits a
> warning on the use of CODE_SYM_START_LOCAL_NOALIGN(.L_restore)
>
> I think it's sufficient.
>
Okay, Thanks.. I will send the modified patch :)
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-22 13:18 ` Aditya
@ 2021-01-22 19:10 ` Joe Perches
2021-01-22 20:13 ` Aditya
0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-01-22 19:10 UTC (permalink / raw)
To: Aditya, linux-kernel
Cc: broonie, linux-kernel-mentees, clang-built-linux, dwaipayanray1
On Fri, 2021-01-22 at 18:48 +0530, Aditya wrote:
> On 21/1/21 12:13 am, Joe Perches wrote:
> > I believe the test should be:
> >
> > if ($realfile =~ /\.S$/ &&
> > $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
>
> Joe, with this regex, we won't be able to match
> "jmp .L_restore
> SYM_FUNC_END(\name)"
I think that's not an issue.
> which was replaced in this patch in the discussion:
> https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/phKe-Tb6CgAJ
>
> Here, "jmp .L_restore" was also replaced to fix the error.
Notice that this line was also replaced in the same patch:
#ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
> However, if this
> regex(/^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) is
> correct, maybe we don't need to check for $file as we are now checking
> for just one line.
>
> What do you think?
The test I wrote was complete, did not use $file and emits a
warning on the use of CODE_SYM_START_LOCAL_NOALIGN(.L_restore)
I think it's sufficient.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-20 18:43 ` Joe Perches
2021-01-20 18:57 ` Nick Desaulniers via Linux-kernel-mentees
@ 2021-01-22 13:18 ` Aditya
2021-01-22 19:10 ` Joe Perches
1 sibling, 1 reply; 21+ messages in thread
From: Aditya @ 2021-01-22 13:18 UTC (permalink / raw)
To: Joe Perches, linux-kernel
Cc: broonie, linux-kernel-mentees, clang-built-linux, dwaipayanray1
On 21/1/21 12:13 am, Joe Perches wrote:
> On Wed, 2021-01-20 at 18:23 +0530, Aditya wrote:
>> On 20/1/21 2:51 pm, Joe Perches wrote:
>>> On Wed, 2021-01-20 at 12:55 +0530, Aditya Srivastava 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 a warning on finding the usage of '.L' symbols
>>>> in '.S' files, if it lies within SYM_*_START/END annotation pair.
>>>
>>> I believe this needs a test for $file as it won't work well on
>>> patches as the SYM_*_START/END may not be in the patch context.
>>>
>> Okay.
>>
>>> Also, is this supposed to work for local labels like '.L<foo>:'?
>>> I don't think a warning should be generated for those.
>>>
>> Yes, currently it will generate warning for all symbols which start
>> with .L and have non- white character symbol following it, if it is
>> lying within SYM_*_START/END annotation pair.
>>
>> Should I reduce the check to \.L_\S+ instead? (please note "_"
>> following ".L")
>
> Use grep first. That would still match several existing labels.
>
>> Pardon me, I'm not good with assembly :/
>
> Spending time reading docs can help with that.
>
> Mark? Can you please comment about the below?
>
> I believe the test should be:
>
> if ($realfile =~ /\.S$/ &&
> $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
Joe, with this regex, we won't be able to match
"jmp .L_restore
SYM_FUNC_END(\name)"
which was replaced in this patch in the discussion:
https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/phKe-Tb6CgAJ
Here, "jmp .L_restore" was also replaced to fix the error.
However, if this
regex(/^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) is
correct, maybe we don't need to check for $file as we are now checking
for just one line.
What do you think?
Thanks
Aditya
> WARN(...);
> }
>
> so that only this code currently matches:
>
> $ git grep -P '^\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L' -- '*.S'
> arch/x86/boot/compressed/head_32.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> arch/x86/boot/compressed/head_32.S:SYM_FUNC_END(.Lrelocated)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lrelocated)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lpaging_enabled)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lpaging_enabled)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmode)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lno_longmode)
> arch/x86/boot/pmjump.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lin_pm32)
> arch/x86/boot/pmjump.S:SYM_FUNC_END(.Lin_pm32)
> arch/x86/entry/entry_64.S:SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
> arch/x86/entry/entry_64.S:SYM_CODE_END(.Lbad_gs)
> arch/x86/lib/copy_user_64.S:SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
> arch/x86/lib/copy_user_64.S:SYM_CODE_END(.Lcopy_user_handle_tail)
> arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
> arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_clac)
> arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_8_clac)
> arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_8_clac)
> arch/x86/lib/putuser.S:SYM_CODE_START_LOCAL(.Lbad_put_user_clac)
> arch/x86/lib/putuser.S:SYM_CODE_END(.Lbad_put_user_clac)
> arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_START_LOCAL(.Lwakeup_idt)
> arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_END(.Lwakeup_idt)
>
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-20 18:57 ` Nick Desaulniers via Linux-kernel-mentees
@ 2021-01-20 22:59 ` Josh Poimboeuf
0 siblings, 0 replies; 21+ messages in thread
From: Josh Poimboeuf @ 2021-01-20 22:59 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Aditya, dwaipayanray1, LKML, clang-built-linux, Mark Brown,
Joe Perches, linux-kernel-mentees
On Wed, Jan 20, 2021 at 10:57:03AM -0800, Nick Desaulniers wrote:
> > $ git grep -P '^\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L' -- '*.S'
> > arch/x86/boot/compressed/head_32.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> > arch/x86/boot/compressed/head_32.S:SYM_FUNC_END(.Lrelocated)
> > arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> > arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lrelocated)
> > arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lpaging_enabled)
> > arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lpaging_enabled)
> > arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmode)
> > arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lno_longmode)
> > arch/x86/boot/pmjump.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lin_pm32)
> > arch/x86/boot/pmjump.S:SYM_FUNC_END(.Lin_pm32)
> > arch/x86/entry/entry_64.S:SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
> > arch/x86/entry/entry_64.S:SYM_CODE_END(.Lbad_gs)
> > arch/x86/lib/copy_user_64.S:SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
> > arch/x86/lib/copy_user_64.S:SYM_CODE_END(.Lcopy_user_handle_tail)
> > arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
> > arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_clac)
> > arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_8_clac)
> > arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_8_clac)
> > arch/x86/lib/putuser.S:SYM_CODE_START_LOCAL(.Lbad_put_user_clac)
> > arch/x86/lib/putuser.S:SYM_CODE_END(.Lbad_put_user_clac)
> > arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_START_LOCAL(.Lwakeup_idt)
> > arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_END(.Lwakeup_idt)
>
> Josh, I assume objtool does not annotate code under:
> arch/x86/boot/
> arch/x86/entry/
> arch/x86/realmode/
> ?
>
> The rest, ie
> arch/x86/lib/
> seem potentially relevant?
Both arch/x86/entry/* and arch/x86/lib/* are read by objtool and should
probably be fixed up.
--
Josh
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-20 18:43 ` Joe Perches
@ 2021-01-20 18:57 ` Nick Desaulniers via Linux-kernel-mentees
2021-01-20 22:59 ` Josh Poimboeuf
2021-01-22 13:18 ` Aditya
1 sibling, 1 reply; 21+ messages in thread
From: Nick Desaulniers via Linux-kernel-mentees @ 2021-01-20 18:57 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Aditya, dwaipayanray1, LKML, clang-built-linux, Mark Brown,
Joe Perches, linux-kernel-mentees
On Wed, Jan 20, 2021 at 10:43 AM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2021-01-20 at 18:23 +0530, Aditya wrote:
> > On 20/1/21 2:51 pm, Joe Perches wrote:
> > > On Wed, 2021-01-20 at 12:55 +0530, Aditya Srivastava 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 a warning on finding the usage of '.L' symbols
> > > > in '.S' files, if it lies within SYM_*_START/END annotation pair.
> > >
> > > I believe this needs a test for $file as it won't work well on
> > > patches as the SYM_*_START/END may not be in the patch context.
> > >
> > Okay.
> >
> > > Also, is this supposed to work for local labels like '.L<foo>:'?
> > > I don't think a warning should be generated for those.
> > >
> > Yes, currently it will generate warning for all symbols which start
> > with .L and have non- white character symbol following it, if it is
> > lying within SYM_*_START/END annotation pair.
> >
> > Should I reduce the check to \.L_\S+ instead? (please note "_"
> > following ".L")
>
> Use grep first. That would still match several existing labels.
>
> > Pardon me, I'm not good with assembly :/
>
> Spending time reading docs can help with that.
>
> Mark? Can you please comment about the below?
>
> I believe the test should be:
>
> if ($realfile =~ /\.S$/ &&
> $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
> WARN(...);
> }
>
> so that only this code currently matches:
>
> $ git grep -P '^\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L' -- '*.S'
> arch/x86/boot/compressed/head_32.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> arch/x86/boot/compressed/head_32.S:SYM_FUNC_END(.Lrelocated)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lrelocated)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lpaging_enabled)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lpaging_enabled)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmode)
> arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lno_longmode)
> arch/x86/boot/pmjump.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lin_pm32)
> arch/x86/boot/pmjump.S:SYM_FUNC_END(.Lin_pm32)
> arch/x86/entry/entry_64.S:SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
> arch/x86/entry/entry_64.S:SYM_CODE_END(.Lbad_gs)
> arch/x86/lib/copy_user_64.S:SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
> arch/x86/lib/copy_user_64.S:SYM_CODE_END(.Lcopy_user_handle_tail)
> arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
> arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_clac)
> arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_8_clac)
> arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_8_clac)
> arch/x86/lib/putuser.S:SYM_CODE_START_LOCAL(.Lbad_put_user_clac)
> arch/x86/lib/putuser.S:SYM_CODE_END(.Lbad_put_user_clac)
> arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_START_LOCAL(.Lwakeup_idt)
> arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_END(.Lwakeup_idt)
Josh, I assume objtool does not annotate code under:
arch/x86/boot/
arch/x86/entry/
arch/x86/realmode/
?
The rest, ie
arch/x86/lib/
seem potentially relevant?
--
Thanks,
~Nick Desaulniers
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-20 12:53 ` Aditya
@ 2021-01-20 18:43 ` Joe Perches
2021-01-20 18:57 ` Nick Desaulniers via Linux-kernel-mentees
2021-01-22 13:18 ` Aditya
0 siblings, 2 replies; 21+ messages in thread
From: Joe Perches @ 2021-01-20 18:43 UTC (permalink / raw)
To: Aditya, linux-kernel
Cc: broonie, linux-kernel-mentees, clang-built-linux, dwaipayanray1
On Wed, 2021-01-20 at 18:23 +0530, Aditya wrote:
> On 20/1/21 2:51 pm, Joe Perches wrote:
> > On Wed, 2021-01-20 at 12:55 +0530, Aditya Srivastava 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 a warning on finding the usage of '.L' symbols
> > > in '.S' files, if it lies within SYM_*_START/END annotation pair.
> >
> > I believe this needs a test for $file as it won't work well on
> > patches as the SYM_*_START/END may not be in the patch context.
> >
> Okay.
>
> > Also, is this supposed to work for local labels like '.L<foo>:'?
> > I don't think a warning should be generated for those.
> >
> Yes, currently it will generate warning for all symbols which start
> with .L and have non- white character symbol following it, if it is
> lying within SYM_*_START/END annotation pair.
>
> Should I reduce the check to \.L_\S+ instead? (please note "_"
> following ".L")
Use grep first. That would still match several existing labels.
> Pardon me, I'm not good with assembly :/
Spending time reading docs can help with that.
Mark? Can you please comment about the below?
I believe the test should be:
if ($realfile =~ /\.S$/ &&
$line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
WARN(...);
}
so that only this code currently matches:
$ git grep -P '^\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L' -- '*.S'
arch/x86/boot/compressed/head_32.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
arch/x86/boot/compressed/head_32.S:SYM_FUNC_END(.Lrelocated)
arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lrelocated)
arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lpaging_enabled)
arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lpaging_enabled)
arch/x86/boot/compressed/head_64.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmode)
arch/x86/boot/compressed/head_64.S:SYM_FUNC_END(.Lno_longmode)
arch/x86/boot/pmjump.S:SYM_FUNC_START_LOCAL_NOALIGN(.Lin_pm32)
arch/x86/boot/pmjump.S:SYM_FUNC_END(.Lin_pm32)
arch/x86/entry/entry_64.S:SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
arch/x86/entry/entry_64.S:SYM_CODE_END(.Lbad_gs)
arch/x86/lib/copy_user_64.S:SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
arch/x86/lib/copy_user_64.S:SYM_CODE_END(.Lcopy_user_handle_tail)
arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_clac)
arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_8_clac)
arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_8_clac)
arch/x86/lib/putuser.S:SYM_CODE_START_LOCAL(.Lbad_put_user_clac)
arch/x86/lib/putuser.S:SYM_CODE_END(.Lbad_put_user_clac)
arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_START_LOCAL(.Lwakeup_idt)
arch/x86/realmode/rm/wakeup_asm.S:SYM_DATA_END(.Lwakeup_idt)
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-20 9:21 ` Joe Perches
@ 2021-01-20 12:53 ` Aditya
2021-01-20 18:43 ` Joe Perches
0 siblings, 1 reply; 21+ messages in thread
From: Aditya @ 2021-01-20 12:53 UTC (permalink / raw)
To: Joe Perches, linux-kernel
Cc: broonie, linux-kernel-mentees, clang-built-linux, dwaipayanray1
On 20/1/21 2:51 pm, Joe Perches wrote:
> On Wed, 2021-01-20 at 12:55 +0530, Aditya Srivastava 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 a warning on finding the usage of '.L' symbols
>> in '.S' files, if it lies within SYM_*_START/END annotation pair.
>
> I believe this needs a test for $file as it won't work well on
> patches as the SYM_*_START/END may not be in the patch context.
>
Okay.
> Also, is this supposed to work for local labels like '.L<foo>:'?
> I don't think a warning should be generated for those.
>
Yes, currently it will generate warning for all symbols which start
with .L and have non- white character symbol following it, if it is
lying within SYM_*_START/END annotation pair.
Should I reduce the check to \.L_\S+ instead? (please note "_"
following ".L")
Pardon me, I'm not good with assembly :/
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
2021-01-20 7:25 Aditya Srivastava
@ 2021-01-20 9:21 ` Joe Perches
2021-01-20 12:53 ` Aditya
0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-01-20 9:21 UTC (permalink / raw)
To: Aditya Srivastava, linux-kernel
Cc: broonie, linux-kernel-mentees, clang-built-linux, dwaipayanray1
On Wed, 2021-01-20 at 12:55 +0530, Aditya Srivastava 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 a warning on finding the usage of '.L' symbols
> in '.S' files, if it lies within SYM_*_START/END annotation pair.
I believe this needs a test for $file as it won't work well on
patches as the SYM_*_START/END may not be in the patch context.
Also, is this supposed to work for local labels like '.L<foo>:'?
I don't think a warning should be generated for those.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files
@ 2021-01-20 7:25 Aditya Srivastava
2021-01-20 9:21 ` Joe Perches
0 siblings, 1 reply; 21+ messages in thread
From: Aditya Srivastava @ 2021-01-20 7:25 UTC (permalink / raw)
To: linux-kernel
Cc: yashsri421, dwaipayanray1, clang-built-linux, broonie, joe,
linux-kernel-mentees
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 a warning on finding the usage of '.L' symbols
in '.S' files, if it lies within SYM_*_START/END annotation pair.
Suggested-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/lkml/20210112210154.GI4646@sirena.org.uk/
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
scripts/checkpatch.pl | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7030c4d6d126..858b5def61e9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2501,6 +2501,9 @@ sub process {
my $checklicenseline = 1;
+ # record SYM_*_START/END annotation pair count, for AVOID_L_PREFIX
+ my $sym_start_block = 0;
+
sanitise_line_reset();
my $line;
foreach my $rawline (@rawlines) {
@@ -3590,6 +3593,25 @@ sub process {
}
}
+# check for .L prefix local symbols in .S files
+ if ($realfile =~ /\.S$/) {
+ if ($line =~ /SYM_.*_START/ ||
+ (defined $context_function && $context_function =~ /SYM_.*_START/)) {
+ $sym_start_block++;
+ }
+
+ if ($line=~ /\.L\S+/ && # line contains .L prefixed local symbol
+ $sym_start_block > 0) { # lies between SYM_*_START and SYM_*_END pair
+ 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);
+ }
+
+ if ($line =~ /SYM_.*_END/ ||
+ (defined $context_function && $context_function =~ /SYM_.*_END/)) {
+ $sym_start_block--;
+ }
+ }
+
# 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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-01-22 20:44 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-16 12:22 [Linux-kernel-mentees] [PATCH] checkpatch: add warning for avoiding .L prefix symbols in assembly files Aditya Srivastava
2021-01-16 12:43 ` Dwaipayan Ray
2021-01-16 13:18 ` Aditya
2021-01-16 13:38 ` Dwaipayan Ray
2021-01-16 14:57 ` Aditya
2021-01-18 5:25 ` Lukas Bulwahn
2021-01-18 14:44 ` Aditya Srivastava
2021-01-18 17:24 ` Lukas Bulwahn
2021-01-18 18:42 ` Aditya
2021-01-18 19:13 ` Aditya Srivastava
2021-01-19 16:32 ` Lukas Bulwahn
2021-01-20 7:25 Aditya Srivastava
2021-01-20 9:21 ` Joe Perches
2021-01-20 12:53 ` Aditya
2021-01-20 18:43 ` Joe Perches
2021-01-20 18:57 ` Nick Desaulniers via Linux-kernel-mentees
2021-01-20 22:59 ` Josh Poimboeuf
2021-01-22 13:18 ` Aditya
2021-01-22 19:10 ` Joe Perches
2021-01-22 20:13 ` Aditya
2021-01-22 20:36 ` Fāng-ruì Sòng via Linux-kernel-mentees
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).