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

[John Stultz]

On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@xxxxxxxxxx> wrote:
> 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?
>

One extra clue on this: I noticed the external module Makefile had
KBUILD_EXTRA_SYMBOLS="$(EXTRA_SYMBOLS)"  in the $(MAKE) string, where
EXTRA_SYMBOLS pointed to some files that no longer exist.  I removed
the KBUILD_EXTRA_SYMBOLS= argument, and magically, the generated
Module.symvers now had tabs at the end of each line.

I wonder if there some path in the KBUILD_EXTRA_SYMBOLS= handling that
isn't generating the output in the same way?

thanks
-john