checkpatch: add warning for avoiding .L prefix symbols in assembly files

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.

Quoted 3 lines · Aditya Srivastava

Local symbols prefixed with '.L' do not emit symbol table entries, as they have special meaning for the assembler.

Quoted 5 lines

'.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.

Quoted 4 lines · Joe Perches

Quoted 3 lines · Aditya Srivastava

Local symbols prefixed with '.L' do not emit symbol table entries, as they have special meaning for the assembler.

Quoted 9 lines

Quoted 5 lines

'.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.

Quoted 3 lines

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 :/

Quoted 19 lines · Aditya

Quoted 4 lines · Joe Perches

Quoted 3 lines · Aditya Srivastava

Local symbols prefixed with '.L' do not emit symbol table entries, as they have special meaning for the assembler.

Quoted 9 lines

Quoted 5 lines

'.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.

Quoted 3 lines

Also, is this supposed to work for local labels like '.L<foo>:'? I don't think a warning should be generated for those.

Quoted 6 lines

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.

Quoted 1 line

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)

Quoted 15 lines

$ 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)

Quoted 8 lines

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?

Quoted 19 lines · Aditya

Quoted 4 lines · Joe Perches

Quoted 3 lines · Aditya Srivastava

Local symbols prefixed with '.L' do not emit symbol table entries, as they have special meaning for the assembler.

Quoted 9 lines

Quoted 5 lines

'.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.

Quoted 3 lines

Also, is this supposed to work for local labels like '.L<foo>:'? I don't think a warning should be generated for those.

Quoted 6 lines

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.

Quoted 1 line

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)

Quoted 4 lines · Joe Perches

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?

Quoted 9 lines · Aditya

Quoted 4 lines · Joe Perches

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.

Quoted 4 lines

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

(quoted diff) +1 -1

-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)

Quoted 7 lines

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.

Quoted 1 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.

Quoted 35 lines · Joe Perches

Quoted 9 lines · Aditya

Quoted 4 lines · Joe Perches

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.

Quoted 4 lines

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

(quoted diff) +1 -1

-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)

Quoted 8 lines

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)

Quoted 2 lines

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

Quoted 7 lines · Aditya Srivastava

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.

Quoted 6 lines

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

Quoted 7 lines

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.

scripts/checkpatch.pl

Quoted 1 line

scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3594

Quoted 7 lines

scripts/checkpatch.pl +0 -0

@@ -3590,6 +3590,13 @@ sub process {
  			}
  		}

scripts/checkpatch.pl +2 -0

+# check for .L prefix local symbols in .S files
+		if ($realfile =~ m@^arch/x86/(?:entry|lib)/.*\.S$@ &&

Using /.S$/ should be enough

scripts/checkpatch.pl sub process { · line 3595

Quoted 1 line

scripts/checkpatch.pl +1 -0

+		    $line =~ /^\+\s*SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {

This might need to be

scripts/checkpatch.pl sub process { · line 3599

Quoted 4 lines

scripts/checkpatch.pl +4 -0

+			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] open here

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(+)

scripts/checkpatch.pl +0 -0

index 7030c4d6d126..4a03326c87b6 100755
@@ -3590,6 +3590,13 @@ sub process {
  			}
  		}

scripts/checkpatch.pl +7 -0

+# 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)$/);
 

Quoted 11 lines · Aditya Srivastava

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.

Quoted 3 lines

Suggested-by Mark Brown <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Signed-off-by Aditya Srivastava <[email protected]>

Acked-by Joe Perches <[email protected]>

Quoted 38 lines

---
* 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(+)

scripts/checkpatch.pl +7 -0

index 7030c4d6d126..4a03326c87b6 100755
@@ -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)$/);