Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1728622 > unrolled thread

Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of copying _shipped files

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-09-08 08:20 +0200
    Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-08 19:30 +0200
      Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-08 20:10 +0200
        Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-08 20:40 +0200
          Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-08 23:40 +0200
            Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Sam Ravnborg <sam@ravnborg.org> - 2017-09-09 08:40 +0200
              Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-09-10 16:10 +0200
            Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-09-10 16:00 +0200
              Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of  copying _shipped files Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-10 18:30 +0200

#1728622 — Re: [RFC PATCH 0/3] kbuild: generate intermediate C files instead of copying _shipped files

Hi Linus,


Very sorry that I had not responded quickly.

When I was digging into tool version dependency,
as you pointed out, gperf is a problem (but seemed one time breakage
for gperf 3.1)
flex seemed very stable for a long time.
bison seemed a bit problem if old version is used.


But, I did not have enough time to take a closer look.
I thought I should respond after I tested more, but I has been pressed
by my daily tasks, then time passed...
Very sorry.



Today, I just noticed gperf usage got dropped from the kernel.

If CONFIG_MODVERSIONS is enabled,
I notice lots of error messages.
WARNING: EXPORT symbol "finish_open" [vmlinux] version generation
failed, symbol will not be


So, I think something was broken in scripts/genksyms/.

Of course, it was a trivial conversion, so it should not be hard to fix...



> gperf is clearly written by clowns that don't understand about
> compatibility issues - it would have been trivial for them to add some
> kind of marker define so that you could test for this directly rather
> than depend on some kind of autoconf "try to build and see if it
> fails" crap.


One idea may be to process the output of "gperf -v"
and embed GPERF_VERSION into the output .c files.

But, if you are unhappy with gperf breakage this time,
we can live without gperf.




> It's likely not even any slower, but who the hell knows.. Do we even
> care? It's almost certainly faster if you compare to generating that
> gperf code.
>

For scripts/kconfig/, I think we do not care at all
because we usually invoke it just once when we configure the build setting.


If we enable CONFIG_MODVERSIONS, scripts/genksyms/ is invoked
over and over again.

Sorry, I have not evaluated if
the perfect hash gives us noticeable advantage or not..



-- 
Best Regards
Masahiro Yamada

[toc] | [next] | [standalone]

#1729111

On Thu, Sep 7, 2017 at 11:18 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> If CONFIG_MODVERSIONS is enabled,
> I notice lots of error messages.
> WARNING: EXPORT symbol "finish_open" [vmlinux] version generation
> failed, symbol will not be versioned
>
> So, I think something was broken in scripts/genksyms/.
>
> Of course, it was a trivial conversion, so it should not be hard to fix...

Indeed, hopefully it would be trivial, but I don't even see the error here.

Of course, I only did a "make allmodconfig" to test the MODVERSIONS
case, I didn't actually install the modules. Is that error perhaps
only detected at install time?

I did build and install a kernel with that patch, but that's my actual
"real" config for the machine, and it didn't have MODVERSIONS enabled.

Oh, how I hate modversions. But I'll take a look if I can see what I
did wrong in the "Trivial and Obvious(tm)" conversion.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1729150

On Fri, Sep 8, 2017 at 10:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Of course, I only did a "make allmodconfig" to test the MODVERSIONS
> case, I didn't actually install the modules. Is that error perhaps
> only detected at install time?

Oh, I take that back. I just got a ton of warnings with my
allmodconfig after doing a "git clean -dqfx".

So I guess there is a dependency issue - my normal build test after
the merge didn't show any issues, simply because the change in
ksymoops didn't actually cause the version information to be
re-generated.

It doesn't seem to happen for every exported symbol, though. Odd.

Looking at it, but not making sense of it yet.

                Linus

[toc] | [prev] | [next] | [standalone]

#1729193

On Fri, Sep 8, 2017 at 11:01 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It doesn't seem to happen for every exported symbol, though. Odd.

Fascinating.

Picking one file at random that shows this, I did net/ceph/mon_client.c.

The version file that gets generated for that looks like this:

__crc_ceph_monc_want_map = 0x3389a57e;
__crc_ceph_monc_got_map = 0x707f45a2;
__crc_ceph_monc_renew_subs = 0x9842030a;
__crc_ceph_monc_wait_osdmap = 0xfe38746d;
__crc_ceph_monc_open_session = 0xc9ba8a19;
__crc_ceph_monc_do_statfs = 0xe878801b;
__crc_ceph_monc_get_version = 0xfaac6ce0;
__crc_ceph_monc_get_version_async = 0x3adefe28;
__crc_ceph_monc_blacklist_add = 0xee71d0ef;
__crc_ceph_monc_init = 0xfce99654;
__crc_ceph_monc_stop = 0xb0d197d0;
__crc_ceph_monc_validate_auth = 0xce1c6d69;

and with the gperf-removal patch, it is 100% identical _except_ that
the "__crc_ceph_monc_do_statfs" line is missing.

Same hashes for everything else. But one missing line. WTF?

What is special about that one particular function vs the other ones
in that file? I have absolutely no idea.

So the really odd thing here is how things clearly still _work_. The
parser works fine for everything else. And looking at the
gprof-removal patch it's not at all obvious how everything could work
fine except for some random thing.

Strange. Does anybody see what the pattern to the failure is?

                Linus

[toc] | [prev] | [next] | [standalone]

#1729301

On Fri, Sep 8, 2017 at 11:39 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Strange. Does anybody see what the pattern to the failure is?

Found it. Stupid special case for 'typeof()' that used
is_reserved_word() in ways I hadn't realized.

Fix committed.

             Linus

[toc] | [prev] | [next] | [standalone]

#1729393

On Fri, Sep 08, 2017 at 02:38:23PM -0700, Linus Torvalds wrote:
> On Fri, Sep 8, 2017 at 11:39 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Strange. Does anybody see what the pattern to the failure is?
> 
> Found it. Stupid special case for 'typeof()' that used
> is_reserved_word() in ways I hadn't realized.
> 
> Fix committed.

To get bonus points for this cleanup you should also remove
the now unused gpref support in Makefile.lib

	Sam

[toc] | [prev] | [next] | [standalone]

#1729979

Hi Sam,

2017-09-09 15:39 GMT+09:00 Sam Ravnborg <sam@ravnborg.org>:
> On Fri, Sep 08, 2017 at 02:38:23PM -0700, Linus Torvalds wrote:
>> On Fri, Sep 8, 2017 at 11:39 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>> >
>> > Strange. Does anybody see what the pattern to the failure is?
>>
>> Found it. Stupid special case for 'typeof()' that used
>> is_reserved_word() in ways I hadn't realized.
>>
>> Fix committed.
>
> To get bonus points for this cleanup you should also remove
> the now unused gpref support in Makefile.lib
>
>         Sam

Yes.

Linus committed c054be10ffd

Thanks!



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1729977

Hi Linus,


2017-09-09 6:38 GMT+09:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Fri, Sep 8, 2017 at 11:39 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Strange. Does anybody see what the pattern to the failure is?
>
> Found it. Stupid special case for 'typeof()' that used
> is_reserved_word() in ways I hadn't realized.
>
> Fix committed.
>
>              Linus


"is_reserved_word()" sounds like a boolean function
that returns 1 or 0.
Maybe, the choice of the function name was not nice.

Anyway, thanks a lot for taking care of all this!



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1730011

On Sun, Sep 10, 2017 at 6:58 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> "is_reserved_word()" sounds like a boolean function
> that returns 1 or 0.
> Maybe, the choice of the function name was not nice.

Yeah, not great name. That's the old name, though - I didn't change
that part, I just changed how it used to return the token structure
pointer, which would be NULL when it wasn't a keyword.

I actually *should* have made it just return 0 for the "not a keyword"
case rather than -1, and that would have ended up being semantically
closer to the old use (because you could treat the return value as a
boolean, like you could with the token pointer). But it's been
literally decades since I used bison/flex, and I didn't remember the
rules for 'enum yytokentype', so I just thought "negative numbers for
error" was safer. Zero would have been fine, no token can have that
number anyway (it just means EOF).

And negative wasn't safer, it caused that bug due to the bare boolean
use I hadn't noticed.

Oh well.

          Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web