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 <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Aditya Srivastava <[email protected]>
---
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
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.
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
On Wed, Jan 20, 2021 at 10:43 AM Joe Perches <[email protected]> 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
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)
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
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)
>
>
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.
On Fri, Jan 22, 2021 at 12:13 PM Aditya <[email protected]> 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?
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
objtool requires that all code must be contained in an ELF symbol.
Symbol names that have a '.L' prefix 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
for '.S' files in arch/x86/entry/* and arch/x86/lib/*, if it denotes
range of code via SYM_*_START/END annotation pair.
Suggested-by: Mark Brown <[email protected]>
Link: https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/4staOlf-CgAJ
Signed-off-by: Aditya Srivastava <[email protected]>
---
* Applies perfectly on next-20210122
Changes in v2:
- Reduce the check to only SYM_*_START/END lines
- Reduce the check for only .S files in arch/x86/entry/* and arch/x86/lib/* as suggested by Josh and Nick
- Modify commit message
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7030c4d6d126..e36cdf96dfe3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3590,6 +3590,13 @@ sub process {
}
}
+# check for .L prefix local symbols in .S files
+ if ($realfile =~ m@^arch/x86/(?:entry|lib)/.*\.S$@ &&
+ $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
+ 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
On Sat, 2021-01-23 at 20:44 +0530, Aditya Srivastava wrote:
> objtool requires that all code must be contained in an ELF symbol.
> Symbol names that have a '.L' prefix 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
> for '.S' files in arch/x86/entry/* and arch/x86/lib/*, if it denotes
> range of code via SYM_*_START/END annotation pair.
>
> Suggested-by: Mark Brown <[email protected]>
> Link: https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/4staOlf-CgAJ
Please do not use groups.google.com links, or if you must, please
use links that are readable.
For instance, this is a better link as it shows the context without
struggling with the poor interface:
https://groups.google.com/g/clang-built-linux/c/E-naBMt_1SM
> Signed-off-by: Aditya Srivastava <[email protected]>
> ---
> * Applies perfectly on next-20210122
>
> Changes in v2:
> - Reduce the check to only SYM_*_START/END lines
> - Reduce the check for only .S files in arch/x86/entry/* and arch/x86/lib/* as suggested by Josh and Nick
I think that's unnecessary.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3590,6 +3590,13 @@ sub process {
> ? }
> ? }
> ?
>
> +# check for .L prefix local symbols in .S files
> + if ($realfile =~ m@^arch/x86/(?:entry|lib)/.*\.S$@ &&
Using /.S$/ should be enough
> + $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
This might need to be
+ $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
> + 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);
> + }
> +
On 23/1/21 10:51 pm, Joe Perches wrote:
> On Sat, 2021-01-23 at 20:44 +0530, Aditya Srivastava wrote:
>> objtool requires that all code must be contained in an ELF symbol.
>> Symbol names that have a '.L' prefix 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
>> for '.S' files in arch/x86/entry/* and arch/x86/lib/*, if it denotes
>> range of code via SYM_*_START/END annotation pair.
>>
>> Suggested-by: Mark Brown <[email protected]>
>> Link: https://groups.google.com/g/clang-built-linux/c/-drkmLgu-cU/m/4staOlf-CgAJ
>
> Please do not use groups.google.com links, or if you must, please
> use links that are readable.
>
> For instance, this is a better link as it shows the context without
> struggling with the poor interface:
>
> https://groups.google.com/g/clang-built-linux/c/E-naBMt_1SM
>
Okay, Got it.. I'll use lkml link for the best.
>> Signed-off-by: Aditya Srivastava <[email protected]>
>> ---
>> * Applies perfectly on next-20210122
>>
>> Changes in v2:
>> - Reduce the check to only SYM_*_START/END lines
>> - Reduce the check for only .S files in arch/x86/entry/* and arch/x86/lib/* as suggested by Josh and Nick
>
> I think that's unnecessary.
>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -3590,6 +3590,13 @@ sub process {
>> }
>> }
>>
>>
>> +# check for .L prefix local symbols in .S files
>> + if ($realfile =~ m@^arch/x86/(?:entry|lib)/.*\.S$@ &&
>
> Using /.S$/ should be enough
>
>> + $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
>
> This might need to be
>
> + $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
>
Okay.. Thanks. I'll modify the patch accordingly.
Thanks
Aditya
>> + 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);
>> + }
>> +
>
>
objtool requires that all code must be contained in an ELF symbol.
Symbol names that have a '.L' prefix 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
for '.S' files, if it denotes range of code via SYM_*_START/END
annotation pair.
Suggested-by: Mark Brown <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
Signed-off-by: Aditya Srivastava <[email protected]>
---
* Applies perfectly on next-20210122
Changes in v3:
- Modify regex for SYM_*_START/END pair
- remove check for arch/x86/entry/* and arch/x86/lib/*
- change 'Link:' in commit message to lkml
- Modify commit description accordingly
Changes in v2:
- Reduce the check to only SYM_*_START/END lines
- Reduce the check for only .S files in arch/x86/entry/* and arch/x86/lib/* as suggested by Josh and Nick
- Modify commit message
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7030c4d6d126..4a03326c87b6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3590,6 +3590,13 @@ sub process {
}
}
+# check for .L prefix local symbols in .S files
+ if ($realfile =~ /\.S$/ &&
+ $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
+ 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
On Sun, 2021-01-24 at 00:34 +0530, Aditya Srivastava wrote:
> objtool requires that all code must be contained in an ELF symbol.
> Symbol names that have a '.L' prefix 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
> for '.S' files, if it denotes range of code via SYM_*_START/END
> annotation pair.
>
> Suggested-by: Mark Brown <[email protected]>
> Link: https://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Aditya Srivastava <[email protected]>
Acked-by: Joe Perches <[email protected]>
> ---
> * Applies perfectly on next-20210122
>
> Changes in v3:
> - Modify regex for SYM_*_START/END pair
> - remove check for arch/x86/entry/* and arch/x86/lib/*
> - change 'Link:' in commit message to lkml
> - Modify commit description accordingly
>
> Changes in v2:
> - Reduce the check to only SYM_*_START/END lines
> - Reduce the check for only .S files in arch/x86/entry/* and arch/x86/lib/* as suggested by Josh and Nick
> - Modify commit message
>
> ?scripts/checkpatch.pl | 7 +++++++
> ?1 file changed, 7 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7030c4d6d126..4a03326c87b6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3590,6 +3590,13 @@ sub process {
> ? }
> ? }
> ?
>
> +# check for .L prefix local symbols in .S files
> + if ($realfile =~ /\.S$/ &&
> + $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
> + 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)$/);
> ?
>
On Sat, Jan 23, 2021 at 1:01 PM Joe Perches <[email protected]> wrote:
>
> On Sun, 2021-01-24 at 00:34 +0530, Aditya Srivastava wrote:
> > objtool requires that all code must be contained in an ELF symbol.
> > Symbol names that have a '.L' prefix 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
> > for '.S' files, if it denotes range of code via SYM_*_START/END
> > annotation pair.
> >
> > Suggested-by: Mark Brown <[email protected]>
> > Link: https://lore.kernel.org/lkml/[email protected]
> > Signed-off-by: Aditya Srivastava <[email protected]>
>
> Acked-by: Joe Perches <[email protected]>
Acked-by: Nick Desaulniers <[email protected]>
Thanks for the patch Aditya!
>
> > ---
> > * Applies perfectly on next-20210122
> >
> > Changes in v3:
> > - Modify regex for SYM_*_START/END pair
> > - remove check for arch/x86/entry/* and arch/x86/lib/*
> > - change 'Link:' in commit message to lkml
> > - Modify commit description accordingly
> >
> > Changes in v2:
> > - Reduce the check to only SYM_*_START/END lines
> > - Reduce the check for only .S files in arch/x86/entry/* and arch/x86/lib/* as suggested by Josh and Nick
> > - Modify commit message
> >
> > scripts/checkpatch.pl | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 7030c4d6d126..4a03326c87b6 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3590,6 +3590,13 @@ sub process {
> > }
> > }
> >
> >
> > +# check for .L prefix local symbols in .S files
> > + if ($realfile =~ /\.S$/ &&
> > + $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
> > + 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)$/);
> >
> >
>
>
--
Thanks,
~Nick Desaulniers