[PATCH 0/2] kernel-doc: fix bug and improve dump_struct

[PATCH 0/2] kernel-doc: fix bug and improve dump_struct

[André Almeida]

Hello,

This patch series improves kernel-doc script at dump_struct()
subroutine by solving a bug and by making the parser more complete.

The current way that scripts/kernel-doc dump structs do not work for
nested structs with attributes (e.g. __packed) and without type alias
(e.g } alias1;). This is due to the way nested structs members are parsed.

Inside dump_struct(), the regex removes attributes and it surrounds
whitespaces. After that, it will do a foreach(id) loop for each alias.
However, we will have an empty string, and then the split function will
return nothing and the foreach will not have any iteration. The code will
then jump the loop and the substitution will fail since $newmember
is uninitialized.

This bug does not happen when the nested struct has no alias and no
attribute, since at process_proto_type() a whitespace is inserted in
order to ensure that the split will not fail. However, with any
attribute, this whitespace will be removed.

This patch solves this by replacing attributes with one whitespace, to
ensure that will have at least one whitespace. 

Besides solving this bug at patch 1, I also add support for the
____cacheline_aligned_in_smp atribute at patch 2.

For testing and reproducing, create a file `code.h`:

/**
 * struct foobar - description
 * @number0: desc0
 * @number1: desc1
 * @number2: desc2
 */
struct foo {
	int number0;

	struct  {
		int number1;
	} __packed;
	
	struct {
		int number2;
	};

 };

I've tested with CRYPTO_MINALIGN_ATTR, __attribute__((__aligned__(8)))
and __aligned() as well. Now, run the `./script/kernel-doc code.h`,
and this is the output:

Use of uninitialized value $newmember in substitution (s///) at
./scripts/kernel-doc line 1152, <IN> line 18.


.. c:type:: struct foo

   description

**Definition**

::

  struct foo {
    int number0;
    struct {
      int number1;
    };
    struct {
      int number2;
    };
  };

**Members**

``number0``
  desc0

``{unnamed_struct}``
  anonymous

``number2``
  desc2

The output will not display the member number1 and will also display an
uninitialized warning. Running after the patch will get rid of the
warning and also display the description for number1:

[...]

**Members**

``number0``
  desc0

``{unnamed_struct}``
  anonymous

``number1``
  desc1

``{unnamed_struct}``
  anonymous

``number2``
  desc2


To test patch [2/2], just replace __packed for
____cacheline_aligned_in_smp and compare how, at the previous stage the
script believes ____cacheline... is an alias for the struct and after
the patch it is removed just as the others attributes.

Finally, I have compared the output of "make htmldocs" with and without
this patch. No new warning were found and one warning regarding
____cacheline_aligned_in_smp at include/linux/netdevice.h was removed.

Thanks,
	André



André Almeida (2):
  kernel-doc: fix processing nested structs with attributes
  kernel-doc: add support for ____cacheline_aligned_in_smp attribute

 scripts/kernel-doc | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.23.0

[PATCH 1/2] kernel-doc: fix processing nested structs with attributes

[André Almeida]

The current regular expression for strip attributes of structs (and
for nested ones as well) also removes all whitespaces that may
surround the attribute. After that, the code will split structs and
iterate for each symbol separated by comma at the end of struct
definition (e.g. "} alias1, alias2;"). However, if the nested struct
does not have any alias and has an attribute, it will result in a
empty string at the closing bracket (e.g "};"). This will make the
split return nothing and $newmember will keep uninitialized. Fix
that, by ensuring that the attribute substitution will leave at least
one whitespace.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 scripts/kernel-doc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 6b03012750da..f1faa036ee59 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1073,10 +1073,10 @@ sub dump_struct($$) {
 	# strip comments:
 	$members =~ s/\/\*.*?\*\///gos;
 	# strip attributes
-	$members =~ s/\s*__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)//gi;
-	$members =~ s/\s*__aligned\s*\([^;]*\)//gos;
-	$members =~ s/\s*__packed\s*//gos;
-	$members =~ s/\s*CRYPTO_MINALIGN_ATTR//gos;
+	$members =~ s/\s*__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)/ /gi;
+	$members =~ s/\s*__aligned\s*\([^;]*\)/ /gos;
+	$members =~ s/\s*__packed\s*/ /gos;
+	$members =~ s/\s*CRYPTO_MINALIGN_ATTR/ /gos;
 	# replace DECLARE_BITMAP
 	$members =~ s/DECLARE_BITMAP\s*\(([^,)]+),\s*([^,)]+)\)/unsigned long $1\[BITS_TO_LONGS($2)\]/gos;
 	# replace DECLARE_HASHTABLE
-- 
2.23.0

[PATCH 2/2] kernel-doc: add support for ____cacheline_aligned_in_smp attribute

[André Almeida]

Subroutine dump_struct uses type attributes to check if the struct
syntax is valid. Then, it removes all attributes before using it for
output. `____cacheline_aligned_in_smp` is an attribute that is
not included in both steps. Add it, since it is used by kernel structs.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 scripts/kernel-doc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index f1faa036ee59..5505750261d7 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1062,7 +1062,7 @@ sub dump_struct($$) {
     my $x = shift;
     my $file = shift;
 
-    if ($x =~ /(struct|union)\s+(\w+)\s*\{(.*)\}(\s*(__packed|__aligned|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\)))*/) {
+    if ($x =~ /(struct|union)\s+(\w+)\s*\{(.*)\}(\s*(__packed|__aligned|____cacheline_aligned_in_smp|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\)))*/) {
 	my $decl_type = $1;
 	$declaration_name = $2;
 	my $members = $3;
@@ -1077,6 +1077,7 @@ sub dump_struct($$) {
 	$members =~ s/\s*__aligned\s*\([^;]*\)/ /gos;
 	$members =~ s/\s*__packed\s*/ /gos;
 	$members =~ s/\s*CRYPTO_MINALIGN_ATTR/ /gos;
+	$members =~ s/\s*____cacheline_aligned_in_smp/ /gos;
 	# replace DECLARE_BITMAP
 	$members =~ s/DECLARE_BITMAP\s*\(([^,)]+),\s*([^,)]+)\)/unsigned long $1\[BITS_TO_LONGS($2)\]/gos;
 	# replace DECLARE_HASHTABLE
-- 
2.23.0

Re: [PATCH 2/2] kernel-doc: add support for ____cacheline_aligned_in_smp attribute

[Jonathan Corbet]

On Tue, 17 Sep 2019 16:41:46 -0300
André Almeida <andrealmeid@collabora.com> wrote:

> -    if ($x =~ /(struct|union)\s+(\w+)\s*\{(.*)\}(\s*(__packed|__aligned|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\)))*/) {
> +    if ($x =~ /(struct|union)\s+(\w+)\s*\{(.*)\}(\s*(__packed|__aligned|____cacheline_aligned_in_smp|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\)))*/) {

Sigh...scripts/kernel-doc just makes me want to cry sometimes...

I've applied both patches.  Thanks, and apologies for the delay...

jon