Re: [PATCH v2] modpost: move the namespace field in Module.symvers last

[John Stultz]

On Wed, Mar 11, 2020 at 10:03 AM Jessica Yu <jeyu@xxxxxxxxxx> wrote:
>
> In order to preserve backwards compatability with kmod tools, we have to
> move the namespace field in Module.symvers last, as the depmod -e -E
> option looks at the first three fields in Module.symvers to check symbol
> versions (and it's expected they stay in the original order of crc,
> symbol, module).
>
> In addition, update an ancient comment above read_dump() in modpost that
> suggested that the export type field in Module.symvers was optional. I
> suspect that there were historical reasons behind that comment that are
> no longer accurate. We have been unconditionally printing the export
> type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
>
> Fix up read_dump() to treat each field as non-optional. I suspect the
> original read_dump() code treated the export field as optional in order
> to support pre <= 2.6.18 Module.symvers (which did not have the export
> type field). Note that although symbol namespaces are optional, the
> field will not be omitted from Module.symvers if a symbol does not have
> a namespace. In this case, the field will simply be empty and the next
> delimiter or end of line will follow.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> Tested-by: Matthias Maennich <maennich@xxxxxxxxxx>
> Reviewed-by: Matthias Maennich <maennich@xxxxxxxxxx>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
> ---
> v2:
>
>   - Explain the changes to read_dump() and the comment (and provide
>     historical context) in the commit message. (Lucas De Marchi)
>
>  Documentation/kbuild/modules.rst |  4 ++--
>  scripts/export_report.pl         |  2 +-
>  scripts/mod/modpost.c            | 24 ++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index 69fa48ee93d6..e0b45a257f21 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -470,9 +470,9 @@ build.
>
>         The syntax of the Module.symvers file is::
>
> -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
> +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
> -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
> +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
>         The fields are separated by tabs and values may be empty (e.g.
>         if no namespace is defined for an exported symbol).

Despite the documentation here claiming the namespace field can be
empty, I'm seeing some trouble with this patch when building external
modules:
  FATAL: parse error in symbol dump file

I've confirmed reverting it make things work again, but its not clear
to me quite yet why.

The only difference I can find is that the Module.symvers in the
external module project doesn't seem to have a tab at the end of each
line (where as Module.symvers for the kernel - which doesn't seem to
have any namespace names - does).

Is there something I need to tweak on the external Kbuild to get
Module.symvers to be generated properly (with the empty tab at the
end) for this new change?
Or does the parser need to be a bit more flexible?

thanks
-john