diff mbox series

kbuild: fixdep: Resync this with v4.17

Message ID 20200218022909.2158-1-trini@konsulko.com
State Superseded
Headers show
Series kbuild: fixdep: Resync this with v4.17 | expand

Commit Message

Tom Rini Feb. 18, 2020, 2:29 a.m. UTC
The previous kbuild resync of e91610da7c8a ("kconfig: re-sync with Linux
4.17-rc4") accidentally did not sync the fixdep program.  This commit
brings fixdep in line with the rest of that previous resync.

This includes all of the following Linux kernel commits:
fbfa9be9904e kbuild: move include/config/ksym/* to include/ksym/*
5b8ad96d1a44 fixdep: remove some false CONFIG_ matches
14a596a7e6fd fixdep: remove stale references to uml-config.h
ab9ce9feed36 fixdep: use existing helper to check modular CONFIG options
87b95a81357d fixdep: refactor parse_dep_file()
5d1ef76f5a22 fixdep: move global variables to local variables of main()
ccfe78873c22 fixdep: remove unneeded memcpy() in parse_dep_file()
4003fd80cba9 fixdep: factor out common code for reading files
01b5cbe7012f fixdep: use malloc() and read() to load dep_file to buffer
41f92cffba19 fixdep: remove unnecessary <arpa/inet.h> inclusion
7c2ec43a2154 fixdep: exit with error code in error branches of do_config_file()
4e433fc4d1a9 fixdep: trivial: typo fix and correction
dee81e988674 fixdep: faster CONFIG_ search
c1a95fda2a40 kbuild: add fine grained build dependencies for exported symbols
d8329e35cc08 fixdep: accept extra dependencies on stdin
4c835b57b8de fixdep: constify strrcmp arguments

Of note is that when applying dee81e988674 above our logic in that area
required some careful consideration to continue to apply.

We specifically do not include:
638e69cf2230 fixdep: do not ignore kconfig.h
as this leads to build problems for us resulting in problems like:
dts/.dt.dtb.o.cmd:5: *** unterminated call to function 'wildcard': missing ')'.  Stop.

Cc: Masahiro Yamada <masahiroy at kernel.org>'
Signed-off-by: Tom Rini <trini at konsulko.com>
---
 scripts/basic/fixdep.c | 352 ++++++++++++++++++-----------------------
 1 file changed, 151 insertions(+), 201 deletions(-)

Comments

Masahiro Yamada Feb. 18, 2020, 11:32 a.m. UTC | #1
Hi Tom,

On Tue, Feb 18, 2020 at 11:29 AM Tom Rini <trini at konsulko.com> wrote:
>
> The previous kbuild resync


Precisely, it was "kconfig resync", not "kbuild resync".


> of e91610da7c8a ("kconfig: re-sync with Linux
> 4.17-rc4") accidentally did not sync the fixdep program.


I do not think it was accidental.

Kconfig and fixdep are separate programs.
You do not necessarily need to sync them together.



> This commit
> brings fixdep in line with the rest of that previous resync.
>
> This includes all of the following Linux kernel commits:
> fbfa9be9904e kbuild: move include/config/ksym/* to include/ksym/*
> 5b8ad96d1a44 fixdep: remove some false CONFIG_ matches
> 14a596a7e6fd fixdep: remove stale references to uml-config.h
> ab9ce9feed36 fixdep: use existing helper to check modular CONFIG options
> 87b95a81357d fixdep: refactor parse_dep_file()
> 5d1ef76f5a22 fixdep: move global variables to local variables of main()
> ccfe78873c22 fixdep: remove unneeded memcpy() in parse_dep_file()
> 4003fd80cba9 fixdep: factor out common code for reading files
> 01b5cbe7012f fixdep: use malloc() and read() to load dep_file to buffer
> 41f92cffba19 fixdep: remove unnecessary <arpa/inet.h> inclusion
> 7c2ec43a2154 fixdep: exit with error code in error branches of do_config_file()
> 4e433fc4d1a9 fixdep: trivial: typo fix and correction
> dee81e988674 fixdep: faster CONFIG_ search
> c1a95fda2a40 kbuild: add fine grained build dependencies for exported symbols
> d8329e35cc08 fixdep: accept extra dependencies on stdin
> 4c835b57b8de fixdep: constify strrcmp arguments
>
> Of note is that when applying dee81e988674 above our logic in that area
> required some careful consideration to continue to apply.

I checked it. The resync looks correct.



> We specifically do not include:
> 638e69cf2230 fixdep: do not ignore kconfig.h
> as this leads to build problems for us resulting in problems like:
> dts/.dt.dtb.o.cmd:5: *** unterminated call to function 'wildcard': missing ')'.  Stop.


This is due to config_enabled(CONFIG_VAL(option##_MODULE))
in include/linux/kconfig.h

This line was unneeded in the first place
since U-Boot does not support module,
but I just wanted to make it aligned with Linux somehow.


I sent two patches.
Please choose either depending your workflow.

[A] http://patchwork.ozlabs.org/patch/1239950/
    (If you want to fix the issue before re-sync)

[B] http://patchwork.ozlabs.org/patch/1239952/
    (If you want to fix the issue after re-sync)


Then, you can import
638e69cf2230 fixdep: do not ignore kconfig.h


Thanks.


>
> Cc: Masahiro Yamada <masahiroy at kernel.org>'
> Signed-off-by: Tom Rini <trini at konsulko.com>
> ---
>  scripts/basic/fixdep.c | 352 ++++++++++++++++++-----------------------
>  1 file changed, 151 insertions(+), 201 deletions(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index da7fb2cd4dde..8c21dd08d9f7 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -25,7 +25,7 @@
>   *
>   * So we play the same trick that "mkdep" played before. We replace
>   * the dependency on autoconf.h by a dependency on every config
> - * option which is mentioned in any of the listed prequisites.
> + * option which is mentioned in any of the listed prerequisites.
>   *
>   * kconfig populates a tree in include/config/ with an empty file
>   * for each config symbol and when the configuration is updated
> @@ -34,7 +34,7 @@
>   * the config symbols are rebuilt.
>   *
>   * So if the user changes his CONFIG_HIS_DRIVER option, only the objects
> - * which depend on "include/linux/config/his/driver.h" will be rebuilt,
> + * which depend on "include/config/his/driver.h" will be rebuilt,
>   * so most likely only his driver ;-)
>   *
>   * The idea above dates, by the way, back to Michael E Chastain, AFAIK.
> @@ -75,15 +75,14 @@
>   * and then basically copies the .<target>.d file to stdout, in the
>   * process filtering out the dependency on autoconf.h and adding
>   * dependencies on include/config/my/option.h for every
> - * CONFIG_MY_OPTION encountered in any of the prequisites.
> + * CONFIG_MY_OPTION encountered in any of the prerequisites.
>   *
>   * It will also filter out all the dependencies on *.ver. We need
>   * to make sure that the generated version checksum are globally up
>   * to date before even starting the recursive build, so it's too late
>   * at this point anyway.
>   *
> - * The algorithm to grep for "CONFIG_..." is bit unusual, but should
> - * be fast ;-) We don't even try to really parse the header files, but
> + * We don't even try to really parse the header files, but
>   * merely grep, i.e. if CONFIG_FOO is mentioned in a comment, it will
>   * be picked up as well. It's not a problem with respect to
>   * correctness, since that can only give too many dependencies, thus
> @@ -94,49 +93,57 @@
>   * (Note: it'd be easy to port over the complete mkdep state machine,
>   *  but I don't think the added complexity is worth it)
>   */
> -/*
> - * Note 2: if somebody writes HELLO_CONFIG_BOOM in a file, it will depend onto
> - * CONFIG_BOOM. This could seem a bug (not too hard to fix), but please do not
> - * fix it! Some UserModeLinux files (look at arch/um/) call CONFIG_BOOM as
> - * UML_CONFIG_BOOM, to avoid conflicts with /usr/include/linux/autoconf.h,
> - * through arch/um/include/uml-config.h; this fixdep "bug" makes sure that
> - * those files will have correct dependencies.
> - */
>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> -#include <sys/mman.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <string.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> -#include <limits.h>
>  #include <ctype.h>
> -#include <arpa/inet.h>
> -
> -#define INT_CONF ntohl(0x434f4e46)
> -#define INT_ONFI ntohl(0x4f4e4649)
> -#define INT_NFIG ntohl(0x4e464947)
> -#define INT_FIG_ ntohl(0x4649475f)
>
> -char *target;
> -char *depfile;
> -char *cmdline;
>  int is_spl_build = 0; /* hack for U-Boot */
>
>  static void usage(void)
>  {
> -       fprintf(stderr, "Usage: fixdep <depfile> <target> <cmdline>\n");
> +       fprintf(stderr, "Usage: fixdep [-e] <depfile> <target> <cmdline>\n");
> +       fprintf(stderr, " -e  insert extra dependencies given on stdin\n");
>         exit(1);
>  }
>
>  /*
> - * Print out the commandline prefixed with cmd_<target filename> :=
> + * Print out a dependency path from a symbol name
>   */
> -static void print_cmdline(void)
> +static void print_dep(const char *m, int slen, const char *dir)
>  {
> -       printf("cmd_%s := %s\n\n", target, cmdline);
> +       int c, i;
> +
> +       printf("    $(wildcard %s/", dir);
> +       for (i = 0; i < slen; i++) {
> +               c = m[i];
> +               if (c == '_')
> +                       c = '/';
> +               else
> +                       c = tolower(c);
> +               putchar(c);
> +       }
> +       printf(".h) \\\n");
> +}
> +
> +static void do_extra_deps(void)
> +{
> +       char buf[80];
> +
> +       while (fgets(buf, sizeof(buf), stdin)) {
> +               int len = strlen(buf);
> +
> +               if (len < 2 || buf[len - 1] != '\n') {
> +                       fprintf(stderr, "fixdep: bad data on stdin\n");
> +                       exit(1);
> +               }
> +               print_dep(buf, len - 1, "include/ksym");
> +       }
>  }
>
>  struct item {
> @@ -198,57 +205,44 @@ static void define_config(const char *name, int len, unsigned int hash)
>  static void use_config(const char *m, int slen)
>  {
>         unsigned int hash = strhash(m, slen);
> -       int c, i;
>
>         if (is_defined_config(m, slen, hash))
>             return;
>
>         define_config(m, slen, hash);
> +       print_dep(m, slen, "include/config");
> +}
>
> -       printf("    $(wildcard include/config/");
> -       for (i = 0; i < slen; i++) {
> -               c = m[i];
> -               if (c == '_')
> -                       c = '/';
> -               else
> -                       c = tolower(c);
> -               putchar(c);
> -       }
> -       printf(".h) \\\n");
> +/* test if s ends in sub */
> +static int str_ends_with(const char *s, int slen, const char *sub)
> +{
> +       int sublen = strlen(sub);
> +
> +       if (sublen > slen)
> +               return 0;
> +
> +       return !memcmp(s + slen - sublen, sub, sublen);
>  }
>
> -static void parse_config_file(const char *map, size_t len)
> +static void parse_config_file(const char *p)
>  {
> -       const int *end = (const int *) (map + len);
> -       /* start at +1, so that p can never be < map */
> -       const int *m   = (const int *) map + 1;
> -       const char *p, *q;
> +       const char *q, *r;
> +       const char *start = p;
>         char tmp_buf[256] = "SPL_"; /* hack for U-Boot */
>
> -       for (; m < end; m++) {
> -               if (*m == INT_CONF) { p = (char *) m  ; goto conf; }
> -               if (*m == INT_ONFI) { p = (char *) m-1; goto conf; }
> -               if (*m == INT_NFIG) { p = (char *) m-2; goto conf; }
> -               if (*m == INT_FIG_) { p = (char *) m-3; goto conf; }
> -               continue;
> -       conf:
> -               if (p > map + len - 7)
> -                       continue;
> -               if (memcmp(p, "CONFIG_", 7))
> +       while ((p = strstr(p, "CONFIG_"))) {
> +               if (p > start && (isalnum(p[-1]) || p[-1] == '_')) {
> +                       p += 7;
>                         continue;
> -               p += 7;
> -               for (q = p; q < map + len; q++) {
> -                       if (!(isalnum(*q) || *q == '_'))
> -                               goto found;
>                 }
> -               continue;
> -
> -       found:
> -               if (!memcmp(q - 7, "_MODULE", 7))
> -                       q -= 7;
> -               if (q - p < 0)
> -                       continue;
> -
> +               p += 7;
> +               q = p;
> +               while (*q && (isalnum(*q) || *q == '_'))
> +                       q++;
> +               if (str_ends_with(p, q - p, "_MODULE"))
> +                       r = q - 7;
> +               else
> +                       r = q;
>                 /*
>                  * U-Boot also handles
>                  *   CONFIG_IS_ENABLED(...)
> @@ -261,69 +255,62 @@ static void parse_config_file(const char *map, size_t len)
>                     (q - p == 9 && !memcmp(p, "IS_MODULE(", 10)) ||
>                     (q - p == 3 && !memcmp(p, "VAL(", 4))) {
>                         p = q + 1;
> -                       for (q = p; q < map + len; q++)
> -                               if (*q == ')')
> -                                       goto found2;
> -                       continue;
> -
> -               found2:
> -                       if (is_spl_build) {
> -                               memcpy(tmp_buf + 4, p, q - p);
> -                               q = tmp_buf + 4 + (q - p);
> +                       while (*q && *q != ')')
> +                               q++;
> +                       r = q;
> +                       if (r > p && is_spl_build) {
> +                               memcpy(tmp_buf + 4, p, r - p);
> +                               r = tmp_buf + 4 + (r - p);
>                                 p = tmp_buf;
>                         }
>                 }
>                 /* end U-Boot hack */
>
> -               use_config(p, q - p);
> +               if (r > p)
> +                       use_config(p, r - p);
> +               p = q;
>         }
>  }
>
> -/* test is s ends in sub */
> -static int strrcmp(char *s, char *sub)
> -{
> -       int slen = strlen(s);
> -       int sublen = strlen(sub);
> -
> -       if (sublen > slen)
> -               return 1;
> -
> -       return memcmp(s + slen - sublen, sub, sublen);
> -}
> -
> -static void do_config_file(const char *filename)
> +static void *read_file(const char *filename)
>  {
>         struct stat st;
>         int fd;
> -       void *map;
> +       char *buf;
>
>         fd = open(filename, O_RDONLY);
>         if (fd < 0) {
> -               fprintf(stderr, "fixdep: error opening config file: ");
> +               fprintf(stderr, "fixdep: error opening file: ");
>                 perror(filename);
>                 exit(2);
>         }
>         if (fstat(fd, &st) < 0) {
> -               fprintf(stderr, "fixdep: error fstat'ing config file: ");
> +               fprintf(stderr, "fixdep: error fstat'ing file: ");
>                 perror(filename);
>                 exit(2);
>         }
> -       if (st.st_size == 0) {
> -               close(fd);
> -               return;
> +       buf = malloc(st.st_size + 1);
> +       if (!buf) {
> +               perror("fixdep: malloc");
> +               exit(2);
>         }
> -       map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> -       if ((long) map == -1) {
> -               perror("fixdep: mmap");
> -               close(fd);
> -               return;
> +       if (read(fd, buf, st.st_size) != st.st_size) {
> +               perror("fixdep: read");
> +               exit(2);
>         }
> +       buf[st.st_size] = '\0';
> +       close(fd);
>
> -       parse_config_file(map, st.st_size);
> -
> -       munmap(map, st.st_size);
> +       return buf;
> +}
>
> -       close(fd);
> +/* Ignore certain dependencies */
> +static int is_ignored_file(const char *s, int len)
> +{
> +       return str_ends_with(s, len, "include/generated/autoconf.h") ||
> +              str_ends_with(s, len, "include/generated/autoksyms.h") ||
> +              str_ends_with(s, len, "include/linux/kconfig.h") ||
> +              str_ends_with(s, len, ".ver");
>  }
>
>  /*
> @@ -331,70 +318,70 @@ static void do_config_file(const char *filename)
>   * assignments are parsed not only by make, but also by the rather simple
>   * parser in scripts/mod/sumversion.c.
>   */
> -static void parse_dep_file(void *map, size_t len)
> +static void parse_dep_file(char *m, const char *target, int insert_extra_deps)
>  {
> -       char *m = map;
> -       char *end = m + len;
>         char *p;
> -       char s[PATH_MAX];
> -       int is_target;
> +       int is_last, is_target;
>         int saw_any_target = 0;
>         int is_first_dep = 0;
> +       void *buf;
>
> -       while (m < end) {
> +       while (1) {
>                 /* Skip any "white space" */
> -               while (m < end && (*m == ' ' || *m == '\\' || *m == '\n'))
> +               while (*m == ' ' || *m == '\\' || *m == '\n')
>                         m++;
> +
> +               if (!*m)
> +                       break;
> +
>                 /* Find next "white space" */
>                 p = m;
> -               while (p < end && *p != ' ' && *p != '\\' && *p != '\n')
> +               while (*p && *p != ' ' && *p != '\\' && *p != '\n')
>                         p++;
> +               is_last = (*p == '\0');
>                 /* Is the token we found a target name? */
>                 is_target = (*(p-1) == ':');
>                 /* Don't write any target names into the dependency file */
>                 if (is_target) {
>                         /* The /next/ file is the first dependency */
>                         is_first_dep = 1;
> -               } else {
> -                       /* Save this token/filename */
> -                       memcpy(s, m, p-m);
> -                       s[p - m] = 0;
> -
> -                       /* Ignore certain dependencies */
> -                       if (strrcmp(s, "include/generated/autoconf.h") &&
> -                           strrcmp(s, "arch/um/include/uml-config.h") &&
> -                           strrcmp(s, "include/linux/kconfig.h") &&
> -                           strrcmp(s, ".ver")) {
> +               } else if (!is_ignored_file(m, p - m)) {
> +                       *p = '\0';
> +
> +                       /*
> +                        * Do not list the source file as dependency, so that
> +                        * kbuild is not confused if a .c file is rewritten
> +                        * into .S or vice versa. Storing it in source_* is
> +                        * needed for modpost to compute srcversions.
> +                        */
> +                       if (is_first_dep) {
>                                 /*
> -                                * Do not list the source file as dependency,
> -                                * so that kbuild is not confused if a .c file
> -                                * is rewritten into .S or vice versa. Storing
> -                                * it in source_* is needed for modpost to
> -                                * compute srcversions.
> +                                * If processing the concatenation of multiple
> +                                * dependency files, only process the first
> +                                * target name, which will be the original
> +                                * source name, and ignore any other target
> +                                * names, which will be intermediate temporary
> +                                * files.
>                                  */
> -                               if (is_first_dep) {
> -                                       /*
> -                                        * If processing the concatenation of
> -                                        * multiple dependency files, only
> -                                        * process the first target name, which
> -                                        * will be the original source name,
> -                                        * and ignore any other target names,
> -                                        * which will be intermediate temporary
> -                                        * files.
> -                                        */
> -                                       if (!saw_any_target) {
> -                                               saw_any_target = 1;
> -                                               printf("source_%s := %s\n\n",
> -                                                       target, s);
> -                                               printf("deps_%s := \\\n",
> -                                                       target);
> -                                       }
> -                                       is_first_dep = 0;
> -                               } else
> -                                       printf("  %s \\\n", s);
> -                               do_config_file(s);
> +                               if (!saw_any_target) {
> +                                       saw_any_target = 1;
> +                                       printf("source_%s := %s\n\n",
> +                                              target, m);
> +                                       printf("deps_%s := \\\n", target);
> +                               }
> +                               is_first_dep = 0;
> +                       } else {
> +                               printf("  %s \\\n", m);
>                         }
> +
> +                       buf = read_file(m);
> +                       parse_config_file(buf);
> +                       free(buf);
>                 }
> +
> +               if (is_last)
> +                       break;
> +
>                 /*
>                  * Start searching for next token immediately after the first
>                  * "whitespace" character that follows this token.
> @@ -407,63 +394,23 @@ static void parse_dep_file(void *map, size_t len)
>                 exit(1);
>         }
>
> +       if (insert_extra_deps)
> +               do_extra_deps();
> +
>         printf("\n%s: $(deps_%s)\n\n", target, target);
>         printf("$(deps_%s):\n", target);
>  }
>
> -static void print_deps(void)
> -{
> -       struct stat st;
> -       int fd;
> -       void *map;
> -
> -       fd = open(depfile, O_RDONLY);
> -       if (fd < 0) {
> -               fprintf(stderr, "fixdep: error opening depfile: ");
> -               perror(depfile);
> -               exit(2);
> -       }
> -       if (fstat(fd, &st) < 0) {
> -               fprintf(stderr, "fixdep: error fstat'ing depfile: ");
> -               perror(depfile);
> -               exit(2);
> -       }
> -       if (st.st_size == 0) {
> -               fprintf(stderr,"fixdep: %s is empty\n",depfile);
> -               close(fd);
> -               return;
> -       }
> -       map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> -       if ((long) map == -1) {
> -               perror("fixdep: mmap");
> -               close(fd);
> -               return;
> -       }
> -
> -       parse_dep_file(map, st.st_size);
> -
> -       munmap(map, st.st_size);
> -
> -       close(fd);
> -}
> -
> -static void traps(void)
> -{
> -       static char test[] __attribute__((aligned(sizeof(int)))) = "CONF";
> -       int *p = (int *)test;
> -
> -       if (*p != INT_CONF) {
> -               fprintf(stderr, "fixdep: sizeof(int) != 4 or wrong endianness? %#x\n",
> -                       *p);
> -               exit(2);
> -       }
> -}
> -
>  int main(int argc, char *argv[])
>  {
> -       traps();
> -
> -       if (argc != 4)
> +       const char *depfile, *target, *cmdline;
> +       int insert_extra_deps = 0;
> +       void *buf;
> +
> +       if (argc == 5 && !strcmp(argv[1], "-e")) {
> +               insert_extra_deps = 1;
> +               argv++;
> +       } else if (argc != 4)
>                 usage();
>
>         depfile = argv[1];
> @@ -474,8 +421,11 @@ int main(int argc, char *argv[])
>         if (!strncmp(target, "spl/", 4) || !strncmp(target, "tpl/", 4))
>                 is_spl_build = 1;
>
> -       print_cmdline();
> -       print_deps();
> +       printf("cmd_%s := %s\n\n", target, cmdline);
> +
> +       buf = read_file(depfile);
> +       parse_dep_file(buf, target, insert_extra_deps);
> +       free(buf);
>
>         return 0;
>  }
> --
> 2.17.1
>
Masahiro Yamada Feb. 18, 2020, 11:37 a.m. UTC | #2
On Tue, Feb 18, 2020 at 8:32 PM Masahiro Yamada <masahiroy at kernel.org> wrote:
>
> Hi Tom,
>
> On Tue, Feb 18, 2020 at 11:29 AM Tom Rini <trini at konsulko.com> wrote:
> >
> > The previous kbuild resync
>
>
> Precisely, it was "kconfig resync", not "kbuild resync".
>
>
> > of e91610da7c8a ("kconfig: re-sync with Linux
> > 4.17-rc4") accidentally did not sync the fixdep program.
>
>
> I do not think it was accidental.
>
> Kconfig and fixdep are separate programs.
> You do not necessarily need to sync them together.
>
>
>
> > This commit
> > brings fixdep in line with the rest of that previous resync.
> >
> > This includes all of the following Linux kernel commits:
> > fbfa9be9904e kbuild: move include/config/ksym/* to include/ksym/*
> > 5b8ad96d1a44 fixdep: remove some false CONFIG_ matches
> > 14a596a7e6fd fixdep: remove stale references to uml-config.h
> > ab9ce9feed36 fixdep: use existing helper to check modular CONFIG options
> > 87b95a81357d fixdep: refactor parse_dep_file()
> > 5d1ef76f5a22 fixdep: move global variables to local variables of main()
> > ccfe78873c22 fixdep: remove unneeded memcpy() in parse_dep_file()
> > 4003fd80cba9 fixdep: factor out common code for reading files
> > 01b5cbe7012f fixdep: use malloc() and read() to load dep_file to buffer
> > 41f92cffba19 fixdep: remove unnecessary <arpa/inet.h> inclusion
> > 7c2ec43a2154 fixdep: exit with error code in error branches of do_config_file()
> > 4e433fc4d1a9 fixdep: trivial: typo fix and correction
> > dee81e988674 fixdep: faster CONFIG_ search
> > c1a95fda2a40 kbuild: add fine grained build dependencies for exported symbols
> > d8329e35cc08 fixdep: accept extra dependencies on stdin
> > 4c835b57b8de fixdep: constify strrcmp arguments
> >
> > Of note is that when applying dee81e988674 above our logic in that area
> > required some careful consideration to continue to apply.
>
> I checked it. The resync looks correct.
>
>
>
> > We specifically do not include:
> > 638e69cf2230 fixdep: do not ignore kconfig.h
> > as this leads to build problems for us resulting in problems like:
> > dts/.dt.dtb.o.cmd:5: *** unterminated call to function 'wildcard': missing ')'.  Stop.
>
>
> This is due to config_enabled(CONFIG_VAL(option##_MODULE))
> in include/linux/kconfig.h
>
> This line was unneeded in the first place
> since U-Boot does not support module,
> but I just wanted to make it aligned with Linux somehow.
>
>
> I sent two patches.
> Please choose either depending your workflow.
>
> [A] http://patchwork.ozlabs.org/patch/1239950/
>     (If you want to fix the issue before re-sync)
>
> [B] http://patchwork.ozlabs.org/patch/1239952/
>     (If you want to fix the issue after re-sync)
>
>
> Then, you can import
> 638e69cf2230 fixdep: do not ignore kconfig.h



BTW, I noticed another problem in fixdep.

I will hold back a fix-up
until your resync work is done.
Tom Rini Feb. 18, 2020, 12:23 p.m. UTC | #3
On Tue, Feb 18, 2020 at 08:37:44PM +0900, Masahiro Yamada wrote:
> On Tue, Feb 18, 2020 at 8:32 PM Masahiro Yamada <masahiroy at kernel.org> wrote:
> >
> > Hi Tom,
> >
> > On Tue, Feb 18, 2020 at 11:29 AM Tom Rini <trini at konsulko.com> wrote:
> > >
> > > The previous kbuild resync
> >
> >
> > Precisely, it was "kconfig resync", not "kbuild resync".

True, I'll re-word.

> >
> >
> > > of e91610da7c8a ("kconfig: re-sync with Linux
> > > 4.17-rc4") accidentally did not sync the fixdep program.
> >
> >
> > I do not think it was accidental.
> >
> > Kconfig and fixdep are separate programs.
> > You do not necessarily need to sync them together.

Yes, but there's not a reason to keep them out of sync either.

> > > This commit
> > > brings fixdep in line with the rest of that previous resync.
> > >
> > > This includes all of the following Linux kernel commits:
> > > fbfa9be9904e kbuild: move include/config/ksym/* to include/ksym/*
> > > 5b8ad96d1a44 fixdep: remove some false CONFIG_ matches
> > > 14a596a7e6fd fixdep: remove stale references to uml-config.h
> > > ab9ce9feed36 fixdep: use existing helper to check modular CONFIG options
> > > 87b95a81357d fixdep: refactor parse_dep_file()
> > > 5d1ef76f5a22 fixdep: move global variables to local variables of main()
> > > ccfe78873c22 fixdep: remove unneeded memcpy() in parse_dep_file()
> > > 4003fd80cba9 fixdep: factor out common code for reading files
> > > 01b5cbe7012f fixdep: use malloc() and read() to load dep_file to buffer
> > > 41f92cffba19 fixdep: remove unnecessary <arpa/inet.h> inclusion
> > > 7c2ec43a2154 fixdep: exit with error code in error branches of do_config_file()
> > > 4e433fc4d1a9 fixdep: trivial: typo fix and correction
> > > dee81e988674 fixdep: faster CONFIG_ search
> > > c1a95fda2a40 kbuild: add fine grained build dependencies for exported symbols
> > > d8329e35cc08 fixdep: accept extra dependencies on stdin
> > > 4c835b57b8de fixdep: constify strrcmp arguments
> > >
> > > Of note is that when applying dee81e988674 above our logic in that area
> > > required some careful consideration to continue to apply.
> >
> > I checked it. The resync looks correct.

Great, thanks!

> > > We specifically do not include:
> > > 638e69cf2230 fixdep: do not ignore kconfig.h
> > > as this leads to build problems for us resulting in problems like:
> > > dts/.dt.dtb.o.cmd:5: *** unterminated call to function 'wildcard': missing ')'.  Stop.
> >
> >
> > This is due to config_enabled(CONFIG_VAL(option##_MODULE))
> > in include/linux/kconfig.h
> >
> > This line was unneeded in the first place
> > since U-Boot does not support module,
> > but I just wanted to make it aligned with Linux somehow.
> >
> >
> > I sent two patches.
> > Please choose either depending your workflow.
> >
> > [A] http://patchwork.ozlabs.org/patch/1239950/
> >     (If you want to fix the issue before re-sync)
> >
> > [B] http://patchwork.ozlabs.org/patch/1239952/
> >     (If you want to fix the issue after re-sync)
> >
> >
> > Then, you can import
> > 638e69cf2230 fixdep: do not ignore kconfig.h

Thanks for looking in to it.

> BTW, I noticed another problem in fixdep.
> 
> I will hold back a fix-up
> until your resync work is done.

OK.  I'm going to try and get at least closer to latest release for the
next branch in U-Boot this week.
diff mbox series

Patch

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index da7fb2cd4dde..8c21dd08d9f7 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -25,7 +25,7 @@ 
  *
  * So we play the same trick that "mkdep" played before. We replace
  * the dependency on autoconf.h by a dependency on every config
- * option which is mentioned in any of the listed prequisites.
+ * option which is mentioned in any of the listed prerequisites.
  *
  * kconfig populates a tree in include/config/ with an empty file
  * for each config symbol and when the configuration is updated
@@ -34,7 +34,7 @@ 
  * the config symbols are rebuilt.
  *
  * So if the user changes his CONFIG_HIS_DRIVER option, only the objects
- * which depend on "include/linux/config/his/driver.h" will be rebuilt,
+ * which depend on "include/config/his/driver.h" will be rebuilt,
  * so most likely only his driver ;-)
  *
  * The idea above dates, by the way, back to Michael E Chastain, AFAIK.
@@ -75,15 +75,14 @@ 
  * and then basically copies the .<target>.d file to stdout, in the
  * process filtering out the dependency on autoconf.h and adding
  * dependencies on include/config/my/option.h for every
- * CONFIG_MY_OPTION encountered in any of the prequisites.
+ * CONFIG_MY_OPTION encountered in any of the prerequisites.
  *
  * It will also filter out all the dependencies on *.ver. We need
  * to make sure that the generated version checksum are globally up
  * to date before even starting the recursive build, so it's too late
  * at this point anyway.
  *
- * The algorithm to grep for "CONFIG_..." is bit unusual, but should
- * be fast ;-) We don't even try to really parse the header files, but
+ * We don't even try to really parse the header files, but
  * merely grep, i.e. if CONFIG_FOO is mentioned in a comment, it will
  * be picked up as well. It's not a problem with respect to
  * correctness, since that can only give too many dependencies, thus
@@ -94,49 +93,57 @@ 
  * (Note: it'd be easy to port over the complete mkdep state machine,
  *  but I don't think the added complexity is worth it)
  */
-/*
- * Note 2: if somebody writes HELLO_CONFIG_BOOM in a file, it will depend onto
- * CONFIG_BOOM. This could seem a bug (not too hard to fix), but please do not
- * fix it! Some UserModeLinux files (look at arch/um/) call CONFIG_BOOM as
- * UML_CONFIG_BOOM, to avoid conflicts with /usr/include/linux/autoconf.h,
- * through arch/um/include/uml-config.h; this fixdep "bug" makes sure that
- * those files will have correct dependencies.
- */
 
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <sys/mman.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
-#include <limits.h>
 #include <ctype.h>
-#include <arpa/inet.h>
-
-#define INT_CONF ntohl(0x434f4e46)
-#define INT_ONFI ntohl(0x4f4e4649)
-#define INT_NFIG ntohl(0x4e464947)
-#define INT_FIG_ ntohl(0x4649475f)
 
-char *target;
-char *depfile;
-char *cmdline;
 int is_spl_build = 0; /* hack for U-Boot */
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: fixdep <depfile> <target> <cmdline>\n");
+	fprintf(stderr, "Usage: fixdep [-e] <depfile> <target> <cmdline>\n");
+	fprintf(stderr, " -e  insert extra dependencies given on stdin\n");
 	exit(1);
 }
 
 /*
- * Print out the commandline prefixed with cmd_<target filename> :=
+ * Print out a dependency path from a symbol name
  */
-static void print_cmdline(void)
+static void print_dep(const char *m, int slen, const char *dir)
 {
-	printf("cmd_%s := %s\n\n", target, cmdline);
+	int c, i;
+
+	printf("    $(wildcard %s/", dir);
+	for (i = 0; i < slen; i++) {
+		c = m[i];
+		if (c == '_')
+			c = '/';
+		else
+			c = tolower(c);
+		putchar(c);
+	}
+	printf(".h) \\\n");
+}
+
+static void do_extra_deps(void)
+{
+	char buf[80];
+
+	while (fgets(buf, sizeof(buf), stdin)) {
+		int len = strlen(buf);
+
+		if (len < 2 || buf[len - 1] != '\n') {
+			fprintf(stderr, "fixdep: bad data on stdin\n");
+			exit(1);
+		}
+		print_dep(buf, len - 1, "include/ksym");
+	}
 }
 
 struct item {
@@ -198,57 +205,44 @@  static void define_config(const char *name, int len, unsigned int hash)
 static void use_config(const char *m, int slen)
 {
 	unsigned int hash = strhash(m, slen);
-	int c, i;
 
 	if (is_defined_config(m, slen, hash))
 	    return;
 
 	define_config(m, slen, hash);
+	print_dep(m, slen, "include/config");
+}
 
-	printf("    $(wildcard include/config/");
-	for (i = 0; i < slen; i++) {
-		c = m[i];
-		if (c == '_')
-			c = '/';
-		else
-			c = tolower(c);
-		putchar(c);
-	}
-	printf(".h) \\\n");
+/* test if s ends in sub */
+static int str_ends_with(const char *s, int slen, const char *sub)
+{
+	int sublen = strlen(sub);
+
+	if (sublen > slen)
+		return 0;
+
+	return !memcmp(s + slen - sublen, sub, sublen);
 }
 
-static void parse_config_file(const char *map, size_t len)
+static void parse_config_file(const char *p)
 {
-	const int *end = (const int *) (map + len);
-	/* start at +1, so that p can never be < map */
-	const int *m   = (const int *) map + 1;
-	const char *p, *q;
+	const char *q, *r;
+	const char *start = p;
 	char tmp_buf[256] = "SPL_"; /* hack for U-Boot */
 
-	for (; m < end; m++) {
-		if (*m == INT_CONF) { p = (char *) m  ; goto conf; }
-		if (*m == INT_ONFI) { p = (char *) m-1; goto conf; }
-		if (*m == INT_NFIG) { p = (char *) m-2; goto conf; }
-		if (*m == INT_FIG_) { p = (char *) m-3; goto conf; }
-		continue;
-	conf:
-		if (p > map + len - 7)
-			continue;
-		if (memcmp(p, "CONFIG_", 7))
+	while ((p = strstr(p, "CONFIG_"))) {
+		if (p > start && (isalnum(p[-1]) || p[-1] == '_')) {
+			p += 7;
 			continue;
-		p += 7;
-		for (q = p; q < map + len; q++) {
-			if (!(isalnum(*q) || *q == '_'))
-				goto found;
 		}
-		continue;
-
-	found:
-		if (!memcmp(q - 7, "_MODULE", 7))
-			q -= 7;
-		if (q - p < 0)
-			continue;
-
+		p += 7;
+		q = p;
+		while (*q && (isalnum(*q) || *q == '_'))
+			q++;
+		if (str_ends_with(p, q - p, "_MODULE"))
+			r = q - 7;
+		else
+			r = q;
 		/*
 		 * U-Boot also handles
 		 *   CONFIG_IS_ENABLED(...)
@@ -261,69 +255,62 @@  static void parse_config_file(const char *map, size_t len)
 		    (q - p == 9 && !memcmp(p, "IS_MODULE(", 10)) ||
 		    (q - p == 3 && !memcmp(p, "VAL(", 4))) {
 			p = q + 1;
-			for (q = p; q < map + len; q++)
-				if (*q == ')')
-					goto found2;
-			continue;
-
-		found2:
-			if (is_spl_build) {
-				memcpy(tmp_buf + 4, p, q - p);
-				q = tmp_buf + 4 + (q - p);
+			while (*q && *q != ')')
+				q++;
+			r = q;
+			if (r > p && is_spl_build) {
+				memcpy(tmp_buf + 4, p, r - p);
+				r = tmp_buf + 4 + (r - p);
 				p = tmp_buf;
 			}
 		}
 		/* end U-Boot hack */
 
-		use_config(p, q - p);
+		if (r > p)
+			use_config(p, r - p);
+		p = q;
 	}
 }
 
-/* test is s ends in sub */
-static int strrcmp(char *s, char *sub)
-{
-	int slen = strlen(s);
-	int sublen = strlen(sub);
-
-	if (sublen > slen)
-		return 1;
-
-	return memcmp(s + slen - sublen, sub, sublen);
-}
-
-static void do_config_file(const char *filename)
+static void *read_file(const char *filename)
 {
 	struct stat st;
 	int fd;
-	void *map;
+	char *buf;
 
 	fd = open(filename, O_RDONLY);
 	if (fd < 0) {
-		fprintf(stderr, "fixdep: error opening config file: ");
+		fprintf(stderr, "fixdep: error opening file: ");
 		perror(filename);
 		exit(2);
 	}
 	if (fstat(fd, &st) < 0) {
-		fprintf(stderr, "fixdep: error fstat'ing config file: ");
+		fprintf(stderr, "fixdep: error fstat'ing file: ");
 		perror(filename);
 		exit(2);
 	}
-	if (st.st_size == 0) {
-		close(fd);
-		return;
+	buf = malloc(st.st_size + 1);
+	if (!buf) {
+		perror("fixdep: malloc");
+		exit(2);
 	}
-	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if ((long) map == -1) {
-		perror("fixdep: mmap");
-		close(fd);
-		return;
+	if (read(fd, buf, st.st_size) != st.st_size) {
+		perror("fixdep: read");
+		exit(2);
 	}
+	buf[st.st_size] = '\0';
+	close(fd);
 
-	parse_config_file(map, st.st_size);
-
-	munmap(map, st.st_size);
+	return buf;
+}
 
-	close(fd);
+/* Ignore certain dependencies */
+static int is_ignored_file(const char *s, int len)
+{
+	return str_ends_with(s, len, "include/generated/autoconf.h") ||
+	       str_ends_with(s, len, "include/generated/autoksyms.h") ||
+	       str_ends_with(s, len, "include/linux/kconfig.h") ||
+	       str_ends_with(s, len, ".ver");
 }
 
 /*
@@ -331,70 +318,70 @@  static void do_config_file(const char *filename)
  * assignments are parsed not only by make, but also by the rather simple
  * parser in scripts/mod/sumversion.c.
  */
-static void parse_dep_file(void *map, size_t len)
+static void parse_dep_file(char *m, const char *target, int insert_extra_deps)
 {
-	char *m = map;
-	char *end = m + len;
 	char *p;
-	char s[PATH_MAX];
-	int is_target;
+	int is_last, is_target;
 	int saw_any_target = 0;
 	int is_first_dep = 0;
+	void *buf;
 
-	while (m < end) {
+	while (1) {
 		/* Skip any "white space" */
-		while (m < end && (*m == ' ' || *m == '\\' || *m == '\n'))
+		while (*m == ' ' || *m == '\\' || *m == '\n')
 			m++;
+
+		if (!*m)
+			break;
+
 		/* Find next "white space" */
 		p = m;
-		while (p < end && *p != ' ' && *p != '\\' && *p != '\n')
+		while (*p && *p != ' ' && *p != '\\' && *p != '\n')
 			p++;
+		is_last = (*p == '\0');
 		/* Is the token we found a target name? */
 		is_target = (*(p-1) == ':');
 		/* Don't write any target names into the dependency file */
 		if (is_target) {
 			/* The /next/ file is the first dependency */
 			is_first_dep = 1;
-		} else {
-			/* Save this token/filename */
-			memcpy(s, m, p-m);
-			s[p - m] = 0;
-
-			/* Ignore certain dependencies */
-			if (strrcmp(s, "include/generated/autoconf.h") &&
-			    strrcmp(s, "arch/um/include/uml-config.h") &&
-			    strrcmp(s, "include/linux/kconfig.h") &&
-			    strrcmp(s, ".ver")) {
+		} else if (!is_ignored_file(m, p - m)) {
+			*p = '\0';
+
+			/*
+			 * Do not list the source file as dependency, so that
+			 * kbuild is not confused if a .c file is rewritten
+			 * into .S or vice versa. Storing it in source_* is
+			 * needed for modpost to compute srcversions.
+			 */
+			if (is_first_dep) {
 				/*
-				 * Do not list the source file as dependency,
-				 * so that kbuild is not confused if a .c file
-				 * is rewritten into .S or vice versa. Storing
-				 * it in source_* is needed for modpost to
-				 * compute srcversions.
+				 * If processing the concatenation of multiple
+				 * dependency files, only process the first
+				 * target name, which will be the original
+				 * source name, and ignore any other target
+				 * names, which will be intermediate temporary
+				 * files.
 				 */
-				if (is_first_dep) {
-					/*
-					 * If processing the concatenation of
-					 * multiple dependency files, only
-					 * process the first target name, which
-					 * will be the original source name,
-					 * and ignore any other target names,
-					 * which will be intermediate temporary
-					 * files.
-					 */
-					if (!saw_any_target) {
-						saw_any_target = 1;
-						printf("source_%s := %s\n\n",
-							target, s);
-						printf("deps_%s := \\\n",
-							target);
-					}
-					is_first_dep = 0;
-				} else
-					printf("  %s \\\n", s);
-				do_config_file(s);
+				if (!saw_any_target) {
+					saw_any_target = 1;
+					printf("source_%s := %s\n\n",
+					       target, m);
+					printf("deps_%s := \\\n", target);
+				}
+				is_first_dep = 0;
+			} else {
+				printf("  %s \\\n", m);
 			}
+
+			buf = read_file(m);
+			parse_config_file(buf);
+			free(buf);
 		}
+
+		if (is_last)
+			break;
+
 		/*
 		 * Start searching for next token immediately after the first
 		 * "whitespace" character that follows this token.
@@ -407,63 +394,23 @@  static void parse_dep_file(void *map, size_t len)
 		exit(1);
 	}
 
+	if (insert_extra_deps)
+		do_extra_deps();
+
 	printf("\n%s: $(deps_%s)\n\n", target, target);
 	printf("$(deps_%s):\n", target);
 }
 
-static void print_deps(void)
-{
-	struct stat st;
-	int fd;
-	void *map;
-
-	fd = open(depfile, O_RDONLY);
-	if (fd < 0) {
-		fprintf(stderr, "fixdep: error opening depfile: ");
-		perror(depfile);
-		exit(2);
-	}
-	if (fstat(fd, &st) < 0) {
-		fprintf(stderr, "fixdep: error fstat'ing depfile: ");
-		perror(depfile);
-		exit(2);
-	}
-	if (st.st_size == 0) {
-		fprintf(stderr,"fixdep: %s is empty\n",depfile);
-		close(fd);
-		return;
-	}
-	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if ((long) map == -1) {
-		perror("fixdep: mmap");
-		close(fd);
-		return;
-	}
-
-	parse_dep_file(map, st.st_size);
-
-	munmap(map, st.st_size);
-
-	close(fd);
-}
-
-static void traps(void)
-{
-	static char test[] __attribute__((aligned(sizeof(int)))) = "CONF";
-	int *p = (int *)test;
-
-	if (*p != INT_CONF) {
-		fprintf(stderr, "fixdep: sizeof(int) != 4 or wrong endianness? %#x\n",
-			*p);
-		exit(2);
-	}
-}
-
 int main(int argc, char *argv[])
 {
-	traps();
-
-	if (argc != 4)
+	const char *depfile, *target, *cmdline;
+	int insert_extra_deps = 0;
+	void *buf;
+
+	if (argc == 5 && !strcmp(argv[1], "-e")) {
+		insert_extra_deps = 1;
+		argv++;
+	} else if (argc != 4)
 		usage();
 
 	depfile = argv[1];
@@ -474,8 +421,11 @@  int main(int argc, char *argv[])
 	if (!strncmp(target, "spl/", 4) || !strncmp(target, "tpl/", 4))
 		is_spl_build = 1;
 
-	print_cmdline();
-	print_deps();
+	printf("cmd_%s := %s\n\n", target, cmdline);
+
+	buf = read_file(depfile);
+	parse_dep_file(buf, target, insert_extra_deps);
+	free(buf);
 
 	return 0;
 }