diff mbox series

[-next] modpost: move some defines to the file head

Message ID 20230712015747.77263-1-wangkefeng.wang@huawei.com
State New
Headers
Series [-next] modpost: move some defines to the file head |

Commit Message

Kefeng Wang July 12, 2023, 1:57 a.m. UTC 
  with "module: Ignore RISC-V mapping symbols too", build error occurs,

scripts/mod/modpost.c: In function ‘is_valid_name’:
scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first use in this function)
  return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);

Fix it by moving the EM_RISCV to the file head, also some other
defines in case of similar problem in the future.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 scripts/mod/modpost.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Masahiro Yamada July 12, 2023, 3:55 p.m. UTC | #1 
+To: Luis Chamberlain, the commiter of the breakage



On Wed, Jul 12, 2023 at 10:44 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
> with "module: Ignore RISC-V mapping symbols too", build error occurs,
>
> scripts/mod/modpost.c: In function ‘is_valid_name’:
> scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first use in this function)
>   return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);
>
> Fix it by moving the EM_RISCV to the file head, also some other
> defines in case of similar problem in the future.



BTW, why is the flag 'is_riscv' needed?


All symbols starting with '$' look special to me.



Why not like this?


       if (str[0] == '$')
                 return true;

       return false;






>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  scripts/mod/modpost.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 7c71429d6502..885cca272eb8 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -60,6 +60,22 @@ static unsigned int nr_unresolved;
>
>  #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
>
> +#ifndef EM_RISCV
> +#define EM_RISCV               243
> +#endif
> +
> +#ifndef R_RISCV_SUB32
> +#define R_RISCV_SUB32          39
> +#endif
> +
> +#ifndef EM_LOONGARCH
> +#define EM_LOONGARCH           258
> +#endif
> +
> +#ifndef R_LARCH_SUB32
> +#define R_LARCH_SUB32          55
> +#endif
> +
>  void __attribute__((format(printf, 2, 3)))
>  modpost_log(enum loglevel loglevel, const char *fmt, ...)
>  {
> @@ -1428,22 +1444,6 @@ static int addend_mips_rel(uint32_t *location, Elf_Rela *r)
>         return 0;
>  }
>
> -#ifndef EM_RISCV
> -#define EM_RISCV               243
> -#endif
> -
> -#ifndef R_RISCV_SUB32
> -#define R_RISCV_SUB32          39
> -#endif
> -
> -#ifndef EM_LOONGARCH
> -#define EM_LOONGARCH           258
> -#endif
> -
> -#ifndef R_LARCH_SUB32
> -#define R_LARCH_SUB32          55
> -#endif
> -
>  static void section_rela(struct module *mod, struct elf_info *elf,
>                          Elf_Shdr *sechdr)
>  {
> --
> 2.41.0
>


--
Best Regards

Masahiro Yamada
Palmer Dabbelt July 12, 2023, 4:28 p.m. UTC | #2 
On Wed, 12 Jul 2023 08:55:23 PDT (-0700), masahiroy@kernel.org wrote:
> +To: Luis Chamberlain, the commiter of the breakage
>
>
>
> On Wed, Jul 12, 2023 at 10:44 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>> with "module: Ignore RISC-V mapping symbols too", build error occurs,
>>
>> scripts/mod/modpost.c: In function ‘is_valid_name’:
>> scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first use in this function)
>>   return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);
>>
>> Fix it by moving the EM_RISCV to the file head, also some other
>> defines in case of similar problem in the future.
>
>
>
> BTW, why is the flag 'is_riscv' needed?
>
>
> All symbols starting with '$' look special to me.
>
>
>
> Why not like this?
>
>
>        if (str[0] == '$')
>                  return true;
>
>        return false;

There's a bit of commentary in the v1 
<https://lore.kernel.org/all/20230707054007.32591-1-palmer@rivosinc.com/>, 
but essentially it's not necessary.  I just wanted to play things safe 
and avoid changing the mapping symbol detection elsewhere in order to 
deal with RISC-V.

IIRC we decided $ was special in RISC-V because there were some other 
ports that behaved that way, but it wasn't universal.  If folks are OK 
treating $-prefixed symbols as special everywhere that's fine with me, I 
just wasn't sure what the right answer was.  

There's also some similar arch-specific-ness with the labels and such in 
here.

>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>  scripts/mod/modpost.c | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> index 7c71429d6502..885cca272eb8 100644
>> --- a/scripts/mod/modpost.c
>> +++ b/scripts/mod/modpost.c
>> @@ -60,6 +60,22 @@ static unsigned int nr_unresolved;
>>
>>  #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
>>
>> +#ifndef EM_RISCV
>> +#define EM_RISCV               243
>> +#endif
>> +
>> +#ifndef R_RISCV_SUB32
>> +#define R_RISCV_SUB32          39
>> +#endif
>> +
>> +#ifndef EM_LOONGARCH
>> +#define EM_LOONGARCH           258
>> +#endif
>> +
>> +#ifndef R_LARCH_SUB32
>> +#define R_LARCH_SUB32          55
>> +#endif
>> +
>>  void __attribute__((format(printf, 2, 3)))
>>  modpost_log(enum loglevel loglevel, const char *fmt, ...)
>>  {
>> @@ -1428,22 +1444,6 @@ static int addend_mips_rel(uint32_t *location, Elf_Rela *r)
>>         return 0;
>>  }
>>
>> -#ifndef EM_RISCV
>> -#define EM_RISCV               243
>> -#endif
>> -
>> -#ifndef R_RISCV_SUB32
>> -#define R_RISCV_SUB32          39
>> -#endif
>> -
>> -#ifndef EM_LOONGARCH
>> -#define EM_LOONGARCH           258
>> -#endif
>> -
>> -#ifndef R_LARCH_SUB32
>> -#define R_LARCH_SUB32          55
>> -#endif
>> -
>>  static void section_rela(struct module *mod, struct elf_info *elf,
>>                          Elf_Shdr *sechdr)
>>  {
>> --
>> 2.41.0
>>
Kefeng Wang July 21, 2023, 9:11 a.m. UTC | #3 
Hi, kindly ping, the build issue still exist in Linux-next.

On 2023/7/13 0:28, Palmer Dabbelt wrote:
> On Wed, 12 Jul 2023 08:55:23 PDT (-0700), masahiroy@kernel.org wrote:
>> +To: Luis Chamberlain, the commiter of the breakage
>>
>>
>>
>> On Wed, Jul 12, 2023 at 10:44 AM Kefeng Wang 
>> <wangkefeng.wang@huawei.com> wrote:
>>>
>>> with "module: Ignore RISC-V mapping symbols too", build error occurs,
>>>
>>> scripts/mod/modpost.c: In function ‘is_valid_name’:
>>> scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first 
>>> use in this function)
>>>   return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);
>>>
>>> Fix it by moving the EM_RISCV to the file head, also some other
>>> defines in case of similar problem in the future.
>>
>>
>>
>> BTW, why is the flag 'is_riscv' needed?
>>
>>
>> All symbols starting with '$' look special to me.
>>
>>
>>
>> Why not like this?
>>
>>
>>        if (str[0] == '$')
>>                  return true;
>>
>>        return false;
> 
> There's a bit of commentary in the v1 
> <https://lore.kernel.org/all/20230707054007.32591-1-palmer@rivosinc.com/>, but essentially it's not necessary.  I just wanted to play things safe and avoid changing the mapping symbol detection elsewhere in order to deal with RISC-V.
> 
> IIRC we decided $ was special in RISC-V because there were some other 
> ports that behaved that way, but it wasn't universal.  If folks are OK 
> treating $-prefixed symbols as special everywhere that's fine with me, I 
> just wasn't sure what the right answer was.
> There's also some similar arch-specific-ness with the labels and such in 
> here.
> 
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>>  scripts/mod/modpost.c | 32 ++++++++++++++++----------------
>>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>> index 7c71429d6502..885cca272eb8 100644
>>> --- a/scripts/mod/modpost.c
>>> +++ b/scripts/mod/modpost.c
>>> @@ -60,6 +60,22 @@ static unsigned int nr_unresolved;
>>>
>>>  #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
>>>
>>> +#ifndef EM_RISCV
>>> +#define EM_RISCV               243
>>> +#endif
>>> +
>>> +#ifndef R_RISCV_SUB32
>>> +#define R_RISCV_SUB32          39
>>> +#endif
>>> +
>>> +#ifndef EM_LOONGARCH
>>> +#define EM_LOONGARCH           258
>>> +#endif
>>> +
>>> +#ifndef R_LARCH_SUB32
>>> +#define R_LARCH_SUB32          55
>>> +#endif
>>> +
>>>  void __attribute__((format(printf, 2, 3)))
>>>  modpost_log(enum loglevel loglevel, const char *fmt, ...)
>>>  {
>>> @@ -1428,22 +1444,6 @@ static int addend_mips_rel(uint32_t *location, 
>>> Elf_Rela *r)
>>>         return 0;
>>>  }
>>>
>>> -#ifndef EM_RISCV
>>> -#define EM_RISCV               243
>>> -#endif
>>> -
>>> -#ifndef R_RISCV_SUB32
>>> -#define R_RISCV_SUB32          39
>>> -#endif
>>> -
>>> -#ifndef EM_LOONGARCH
>>> -#define EM_LOONGARCH           258
>>> -#endif
>>> -
>>> -#ifndef R_LARCH_SUB32
>>> -#define R_LARCH_SUB32          55
>>> -#endif
>>> -
>>>  static void section_rela(struct module *mod, struct elf_info *elf,
>>>                          Elf_Shdr *sechdr)
>>>  {
>>> -- 
>>> 2.41.0
>>>
Masahiro Yamada July 21, 2023, 11:58 a.m. UTC | #4 
On Thu, Jul 13, 2023 at 1:28 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Wed, 12 Jul 2023 08:55:23 PDT (-0700), masahiroy@kernel.org wrote:
> > +To: Luis Chamberlain, the commiter of the breakage
> >
> >
> >
> > On Wed, Jul 12, 2023 at 10:44 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >>
> >> with "module: Ignore RISC-V mapping symbols too", build error occurs,
> >>
> >> scripts/mod/modpost.c: In function ‘is_valid_name’:
> >> scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first use in this function)
> >>   return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);
> >>
> >> Fix it by moving the EM_RISCV to the file head, also some other
> >> defines in case of similar problem in the future.
> >
> >
> >
> > BTW, why is the flag 'is_riscv' needed?
> >
> >
> > All symbols starting with '$' look special to me.
> >
> >
> >
> > Why not like this?
> >
> >
> >        if (str[0] == '$')
> >                  return true;
> >
> >        return false;
>
> There's a bit of commentary in the v1
> <https://lore.kernel.org/all/20230707054007.32591-1-palmer@rivosinc.com/>,
> but essentially it's not necessary.  I just wanted to play things safe
> and avoid changing the mapping symbol detection elsewhere in order to
> deal with RISC-V.
>
> IIRC we decided $ was special in RISC-V because there were some other
> ports that behaved that way, but it wasn't universal.  If folks are OK
> treating $-prefixed symbols as special everywhere that's fine with me, I
> just wasn't sure what the right answer was.
>
> There's also some similar arch-specific-ness with the labels and such in
> here.

Hi Palmer,

I am not a toolchain expert, but my gut feeling is
that the code was safer than needed.


I'd like to remove the 'is_riscv' switch rather than
applying this patch.

Will you send a patch, or do you want me to do so?




--
Best Regards
Masahiro Yamada
Palmer Dabbelt July 21, 2023, 2:01 p.m. UTC | #5 
On Fri, 21 Jul 2023 04:58:20 PDT (-0700), masahiroy@kernel.org wrote:
> On Thu, Jul 13, 2023 at 1:28 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> On Wed, 12 Jul 2023 08:55:23 PDT (-0700), masahiroy@kernel.org wrote:
>> > +To: Luis Chamberlain, the commiter of the breakage
>> >
>> >
>> >
>> > On Wed, Jul 12, 2023 at 10:44 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>> >>
>> >> with "module: Ignore RISC-V mapping symbols too", build error occurs,
>> >>
>> >> scripts/mod/modpost.c: In function ‘is_valid_name’:
>> >> scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first use in this function)
>> >>   return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);
>> >>
>> >> Fix it by moving the EM_RISCV to the file head, also some other
>> >> defines in case of similar problem in the future.
>> >
>> >
>> >
>> > BTW, why is the flag 'is_riscv' needed?
>> >
>> >
>> > All symbols starting with '$' look special to me.
>> >
>> >
>> >
>> > Why not like this?
>> >
>> >
>> >        if (str[0] == '$')
>> >                  return true;
>> >
>> >        return false;
>>
>> There's a bit of commentary in the v1
>> <https://lore.kernel.org/all/20230707054007.32591-1-palmer@rivosinc.com/>,
>> but essentially it's not necessary.  I just wanted to play things safe
>> and avoid changing the mapping symbol detection elsewhere in order to
>> deal with RISC-V.
>>
>> IIRC we decided $ was special in RISC-V because there were some other
>> ports that behaved that way, but it wasn't universal.  If folks are OK
>> treating $-prefixed symbols as special everywhere that's fine with me, I
>> just wasn't sure what the right answer was.
>>
>> There's also some similar arch-specific-ness with the labels and such in
>> here.
>
> Hi Palmer,
>
> I am not a toolchain expert, but my gut feeling is
> that the code was safer than needed.
>
>
> I'd like to remove the 'is_riscv' switch rather than
> applying this patch.
>
> Will you send a patch, or do you want me to do so?

I've pretty much got it already.  Do you want it on top of the original 
patch, or just squashed in so you can drop it?
Masahiro Yamada July 21, 2023, 2:19 p.m. UTC | #6 
On Fri, Jul 21, 2023 at 11:01 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Fri, 21 Jul 2023 04:58:20 PDT (-0700), masahiroy@kernel.org wrote:
> > On Thu, Jul 13, 2023 at 1:28 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> >>
> >> On Wed, 12 Jul 2023 08:55:23 PDT (-0700), masahiroy@kernel.org wrote:
> >> > +To: Luis Chamberlain, the commiter of the breakage
> >> >
> >> >
> >> >
> >> > On Wed, Jul 12, 2023 at 10:44 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >> >>
> >> >> with "module: Ignore RISC-V mapping symbols too", build error occurs,
> >> >>
> >> >> scripts/mod/modpost.c: In function ‘is_valid_name’:
> >> >> scripts/mod/modpost.c:1055:57: error: ‘EM_RISCV’ undeclared (first use in this function)
> >> >>   return !is_mapping_symbol(name, elf->hdr->e_machine == EM_RISCV);
> >> >>
> >> >> Fix it by moving the EM_RISCV to the file head, also some other
> >> >> defines in case of similar problem in the future.
> >> >
> >> >
> >> >
> >> > BTW, why is the flag 'is_riscv' needed?
> >> >
> >> >
> >> > All symbols starting with '$' look special to me.
> >> >
> >> >
> >> >
> >> > Why not like this?
> >> >
> >> >
> >> >        if (str[0] == '$')
> >> >                  return true;
> >> >
> >> >        return false;
> >>
> >> There's a bit of commentary in the v1
> >> <https://lore.kernel.org/all/20230707054007.32591-1-palmer@rivosinc.com/>,
> >> but essentially it's not necessary.  I just wanted to play things safe
> >> and avoid changing the mapping symbol detection elsewhere in order to
> >> deal with RISC-V.
> >>
> >> IIRC we decided $ was special in RISC-V because there were some other
> >> ports that behaved that way, but it wasn't universal.  If folks are OK
> >> treating $-prefixed symbols as special everywhere that's fine with me, I
> >> just wasn't sure what the right answer was.
> >>
> >> There's also some similar arch-specific-ness with the labels and such in
> >> here.
> >
> > Hi Palmer,
> >
> > I am not a toolchain expert, but my gut feeling is
> > that the code was safer than needed.
> >
> >
> > I'd like to remove the 'is_riscv' switch rather than
> > applying this patch.
> >
> > Will you send a patch, or do you want me to do so?
>
> I've pretty much got it already.  Do you want it on top of the original
> patch, or just squashed in so you can drop it?



It is up to Luis Chamberlain.

The original patch does not exist in my kbuild tree.
(and I was not even not CC'ed, so I had not noticed it
before I saw this report)




commit c05780ef3c190c2dafbf0be8e65d4f01103ad577
Author:     Palmer Dabbelt <palmer@rivosinc.com>
AuthorDate: Fri Jul 7 09:00:51 2023 -0700
Commit:     Luis Chamberlain <mcgrof@kernel.org>
CommitDate: Mon Jul 10 12:45:23 2023 -0700

    module: Ignore RISC-V mapping symbols too

    RISC-V has an extended form of mapping symbols that we use to encode
    the ISA when it changes in the middle of an ELF.  This trips up modpost
    as a build failure, I haven't yet verified it yet but I believe the
    kallsyms difference should result in stacks looking sane again.

    Reported-by: Randy Dunlap <rdunlap@infradead.org>
    Closes: https://lore.kernel.org/all/9d9e2902-5489-4bf0-d9cb-556c8e5d71c2@infradead.org/
    Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
    Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
    Tested-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
    Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>




--
Best Regards
Masahiro Yamada
diff mbox series

Patch

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7c71429d6502..885cca272eb8 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -60,6 +60,22 @@  static unsigned int nr_unresolved;
 
 #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
 
+#ifndef EM_RISCV
+#define EM_RISCV		243
+#endif
+
+#ifndef R_RISCV_SUB32
+#define R_RISCV_SUB32		39
+#endif
+
+#ifndef EM_LOONGARCH
+#define EM_LOONGARCH		258
+#endif
+
+#ifndef R_LARCH_SUB32
+#define R_LARCH_SUB32		55
+#endif
+
 void __attribute__((format(printf, 2, 3)))
 modpost_log(enum loglevel loglevel, const char *fmt, ...)
 {
@@ -1428,22 +1444,6 @@  static int addend_mips_rel(uint32_t *location, Elf_Rela *r)
 	return 0;
 }
 
-#ifndef EM_RISCV
-#define EM_RISCV		243
-#endif
-
-#ifndef R_RISCV_SUB32
-#define R_RISCV_SUB32		39
-#endif
-
-#ifndef EM_LOONGARCH
-#define EM_LOONGARCH		258
-#endif
-
-#ifndef R_LARCH_SUB32
-#define R_LARCH_SUB32		55
-#endif
-
 static void section_rela(struct module *mod, struct elf_info *elf,
 			 Elf_Shdr *sechdr)
 {