Skip to contentSkip to conversation

fix dependency generation

PATCH In review Jan Beulich 5 participants 14 messages Mar 29, 2007 – Apr 1, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fix dependency generation

Commit 2e3646e51b2d6415549b310655df63e7e0d7a080 changed the way the split config tree is built, but failed to also adjust fixdep accordingly - if changing a config option from or to m, files referencing the respective CONFIG_..._MODULE (but not the corresponding CONFIG_...) didn't get rebuilt.

Once at it, also eliminate false dependencies due to use of ...CONFIG_... identifiers.

scripts/basic/fixdep.c +6 -3
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.
+ * e.g. include/config/his/driver.h, consiting of empty files.
  *
  * 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,
@@ -223,7 +222,7 @@ void use_config(char *m, int slen)
 void parse_config_file(char *map, size_t len)
 {
 	int *end = (int *) (map + len);
-	/* start at +1, so that p can never be < map */
+	/* start at +1, so that p can never be <= map */
 	int *m   = (int *) map + 1;
 	char *p, *q;
 
@@ -235,6 +234,8 @@ void parse_config_file(char *map, size_t
 		continue;
 	conf:
 		if (p > map + len - 7)
+			break;
+		if (isalnum(p[-1]) || p[-1] == '_')
 			continue;
 		if (memcmp(p, "CONFIG_", 7))
 			continue;
@@ -245,6 +246,8 @@ void parse_config_file(char *map, size_t
 		continue;
 
 	found:
+		if (!memcmp(q - 7, "_MODULE", 7))
+			q -= 7;
 		use_config(p+7, q-p-7);
 	}
 }
#1 lore raw
Quoted 14 lines · On Thu, 29 Mar 2007 10:27:14 +0100 Jan Beulich wrote:

Commit 2e3646e51b2d6415549b310655df63e7e0d7a080 changed the way the split config tree is built, but failed to also adjust fixdep accordingly - if changing a config option from or to m, files referencing the respective CONFIG_..._MODULE (but not the corresponding CONFIG_...) didn't get rebuilt.

Once at it, also eliminate false dependencies due to use of ...CONFIG_... identifiers.

scripts/basic/fixdep.c line 33

Quoted 5 lines
scripts/basic/fixdep.c +0 -2
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.

I don't see why you deleted the line above.

scripts/basic/fixdep.c line 32

Quoted 1 line
scripts/basic/fixdep.c +1 -0
+ * e.g. include/config/his/driver.h, consiting of empty files.

consisting

Quoted 22 lines
scripts/basic/fixdep.c +3 -1
  *
  * 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,
@@ -223,7 +222,7 @@ void use_config(char *m, int slen)
 void parse_config_file(char *map, size_t len)
 {
 	int *end = (int *) (map + len);
-	/* start at +1, so that p can never be < map */
+	/* start at +1, so that p can never be <= map */
 	int *m   = (int *) map + 1;
 	char *p, *q;
 
@@ -235,6 +234,8 @@ void parse_config_file(char *map, size_t
 		continue;
 	conf:
 		if (p > map + len - 7)
+			break;
+		if (isalnum(p[-1]) || p[-1] == '_')
 			continue;
 		if (memcmp(p, "CONFIG_", 7))
 			continue;

scripts/basic/fixdep.c void parse_config_file(char *map, size_t · line 253

Quoted 8 lines
scripts/basic/fixdep.c +2 -0
@@ -245,6 +246,8 @@ void parse_config_file(char *map, size_t
 		continue;
 
 	found:
+		if (!memcmp(q - 7, "_MODULE", 7))
+			q -= 7;
 		use_config(p+7, q-p-7);
 	}
 }

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

#2 lore raw
Quoted 11 lines

scripts/basic/fixdep.c line 33

Quoted 9 lines
Quoted 1 line

Randy Dunlap <[email protected]> 29.03.07 17:39 >>>

scripts/basic/fixdep.c +0 -2
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.

I don't see why you deleted the line above.

Because it is no longer true. These files are empty as of 2.6.18.

Jan

#3 lore raw
Quoted 12 lines · On Thu, 29 Mar 2007 17:06:24 +0100 Jan Beulich wrote:
Quoted 11 lines

scripts/basic/fixdep.c line 33

Quoted 9 lines
Quoted 1 line

Randy Dunlap <[email protected]> 29.03.07 17:39 >>>

scripts/basic/fixdep.c +0 -2
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.

I don't see why you deleted the line above.

Quoted 1 line

Because it is no longer true. These files are empty as of 2.6.18.

We seem to be talking about different lines above. Yes, the files are empty, but they are named based on the CONFIG_symbol name, which is what I was trying to get at. So how about a comment like this:

* To be exact, split-include populates a tree in include/config/,
* e.g., include/config/sysctl/syscall.h,
* for the CONFIG_SYSCTL_SYSCALL option, when that option
* is enabled (=y or =m).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

#4 lore raw
Quoted 17 lines
Quoted 1 line
Quoted 1 line

Randy Dunlap <[email protected]> 29.03.07 18:38 >>>

Quoted 12 lines · On Thu, 29 Mar 2007 17:06:24 +0100 Jan Beulich wrote:
Quoted 11 lines

scripts/basic/fixdep.c line 33

Quoted 9 lines
Quoted 1 line

Randy Dunlap <[email protected]> 29.03.07 17:39 >>>

scripts/basic/fixdep.c +0 -2
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.

I don't see why you deleted the line above.

Quoted 1 line

Because it is no longer true. These files are empty as of 2.6.18.

Quoted 8 lines

We seem to be talking about different lines above. Yes, the files are empty, but they are named based on the CONFIG_symbol name, which is what I was trying to get at. So how about a comment like this:

* To be exact, split-include populates a tree in include/config/,
* e.g., include/config/sysctl/syscall.h,
* for the CONFIG_SYSCTL_SYSCALL option, when that option
* is enabled (=y or =m).

Shouldn't that then be '..., when that option is or ever was enabled (=y or =m) since last cleaning the tree'?

Jan

#5 lore raw
Quoted 18 lines · Jan Beulich
Quoted 18 lines
Quoted 1 line
Quoted 1 line

Randy Dunlap <[email protected]> 29.03.07 18:38 >>>

Quoted 11 lines · On Thu, 29 Mar 2007 17:06:24 +0100 Jan Beulich wrote:
Quoted 10 lines

scripts/basic/fixdep.c line 33

Quoted 9 lines
Quoted 1 line

Randy Dunlap <[email protected]> 29.03.07 17:39 >>>

scripts/basic/fixdep.c +0 -2
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.

I don't see why you deleted the line above.

Because it is no longer true. These files are empty as of 2.6.18.

We seem to be talking about different lines above. Yes, the files are empty, but they are named based on the CONFIG_symbol name, which is what I was trying to get at. So how about a comment like this:

Quoted 7 lines
Quoted 4 lines

* To be exact, split-include populates a tree in include/config/,
* e.g., include/config/sysctl/syscall.h,
* for the CONFIG_SYSCTL_SYSCALL option, when that option
* is enabled (=y or =m).

Shouldn't that then be '..., when that option is or ever was enabled (=y or =m) since last cleaning the tree'?

Uh, I have no idea, but I guess that's OK with me.

Signature · 1 line
~Randy
#6 lore raw
In reply to Jan Beulich
Quoted 5 lines · Jan Beulich

Commit 2e3646e51b2d6415549b310655df63e7e0d7a080 changed the way the split config tree is built, but failed to also adjust fixdep accordingly - if changing a config option from or to m, files referencing the respective CONFIG_..._MODULE (but not the corresponding CONFIG_...) didn't get rebuilt.

Do you have a test case for this?
I want to play a little with this before I submit it.

Quoted 3 lines

Once at it, also eliminate false dependencies due to use of ...CONFIG_... identifiers.

But that will break UM - no??
See following note from fixdep:
* 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.

Sam

Quoted 44 lines
scripts/basic/fixdep.c +6 -3
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.
+ * e.g. include/config/his/driver.h, consiting of empty files.
  *
  * 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,
@@ -223,7 +222,7 @@ void use_config(char *m, int slen)
 void parse_config_file(char *map, size_t len)
 {
 	int *end = (int *) (map + len);
-	/* start at +1, so that p can never be < map */
+	/* start at +1, so that p can never be <= map */
 	int *m   = (int *) map + 1;
 	char *p, *q;
 
@@ -235,6 +234,8 @@ void parse_config_file(char *map, size_t
 		continue;
 	conf:
 		if (p > map + len - 7)
+			break;
+		if (isalnum(p[-1]) || p[-1] == '_')
 			continue;
 		if (memcmp(p, "CONFIG_", 7))
 			continue;
@@ -245,6 +246,8 @@ void parse_config_file(char *map, size_t
 		continue;
 
 	found:
+		if (!memcmp(q - 7, "_MODULE", 7))
+			q -= 7;
 		use_config(p+7, q-p-7);
 	}
 }
#7 lore raw
Quoted 9 lines
Quoted 1 line
Quoted 1 line

Sam Ravnborg <[email protected]> 30.03.07 17:08 >>>

Quoted 5 lines · Jan Beulich

Commit 2e3646e51b2d6415549b310655df63e7e0d7a080 changed the way the split config tree is built, but failed to also adjust fixdep accordingly - if changing a config option from or to m, files referencing the respective CONFIG_..._MODULE (but not the corresponding CONFIG_...) didn't get rebuilt.

Do you have a test case for this?
I want to play a little with this before I submit it.

On i386, set CONFIG_APM=y, build, then change it to m.

Quoted 11 lines
Quoted 3 lines

Once at it, also eliminate false dependencies due to use of ...CONFIG_... identifiers.

But that will break UM - no??
See following note from fixdep:
* 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.

Hmm, didn't see this note. Then this might warrant special casing UML, but penalizing all code due to this seems at least odd to me.

Jan

#8 lore raw
Quoted 11 lines · Jan Beulich
Quoted 8 lines

But that will break UM - no??
See following note from fixdep:
* 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.

Hmm, didn't see this note. Then this might warrant special casing UML, but penalizing all code due to this seems at least odd to me.

If I understand this, I would think that special-casing UML_CONFIG_* instead of *_CONFIG_* would be the way to go.

Jeff

Signature · 1 line
Work email - jdike at linux dot intel dot com
#9 lore raw
In reply to Jan Beulich
Quoted 5 lines · Jan Beulich

Commit 2e3646e51b2d6415549b310655df63e7e0d7a080 changed the way the split config tree is built, but failed to also adjust fixdep accordingly - if changing a config option from or to m, files referencing the respective CONFIG_..._MODULE (but not the corresponding CONFIG_...) didn't get rebuilt.

The problem is that tristate symbol represent three values. =n => CONFIG_SYMBOL is undefined
=y => CONFIG_SYMBOL is defined
=m => COMFIG_SYMBOL_MODULE is defined

The function split_config does not take into account the different values and 'fixing' this in fixdep is wrong. Because fixdep does not know if the variable is a tristate symbol or not so it can either blindly remove _MODULE (your patch)
or each time it encounters _MODULE check for a symbol with and without _MODULE.

The better fix is to teach the split_config function that for tristate symbols two files shall be created in the include/config hirachy. So for apm this gets:
include/config/apm.h
include/config/apm/module.h

This will make kconfig behave correct the day that someone add a config symbol with a _MODULE suffix.

I will follow-up with two patches that implement the changes to split_config. The first is a pure code refactoring preparing for the second patch.

Roman - please ack/nack these this since they touches kconfig backend.

Sam

Quoted 47 lines

Once at it, also eliminate false dependencies due to use of ...CONFIG_... identifiers.

scripts/basic/fixdep.c +6 -3
@@ -29,8 +29,7 @@
  * option which is mentioned in any of the listed prequisites.
  *
  * To be exact, split-include populates a tree in include/config/,
- * e.g. include/config/his/driver.h, which contains the #define/#undef
- * for the CONFIG_HIS_DRIVER option.
+ * e.g. include/config/his/driver.h, consiting of empty files.
  *
  * 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,
@@ -223,7 +222,7 @@ void use_config(char *m, int slen)
 void parse_config_file(char *map, size_t len)
 {
 	int *end = (int *) (map + len);
-	/* start at +1, so that p can never be < map */
+	/* start at +1, so that p can never be <= map */
 	int *m   = (int *) map + 1;
 	char *p, *q;
 
@@ -235,6 +234,8 @@ void parse_config_file(char *map, size_t
 		continue;
 	conf:
 		if (p > map + len - 7)
+			break;
+		if (isalnum(p[-1]) || p[-1] == '_')
 			continue;
 		if (memcmp(p, "CONFIG_", 7))
 			continue;
@@ -245,6 +246,8 @@ void parse_config_file(char *map, size_t
 		continue;
 
 	found:
+		if (!memcmp(q - 7, "_MODULE", 7))
+			q -= 7;
 		use_config(p+7, q-p-7);
 	}
 }
#10 lore raw

PATCH 1/2 kconfig: factor out code in conf_spilt_config

Quoted 1 line

From e9fcc3bf8d1c71df1ae650d5291c5d4b15d71656 Mon Sep 17 00:00:00 2001

From: Sam Ravnborg <[email protected]>
Date: Sat, 31 Mar 2007 09:15:12 +0200
Subject: [PATCH] kconfig: factor out code in conf_spilt_config

This patch simply factor out code and do not introduce any functional changes.

---
 scripts/kconfig/confdata.c |   75 ++++++++++++++++++++++++--------------------
 1 files changed, 41 insertions(+), 34 deletions(-)
scripts/kconfig/confdata.c +41 -34
index 664fe29..ff6b39b 100644
@@ -533,13 +533,48 @@ int conf_write(const char *name)
 	return 0;
 }
 
+/* Touch the file specified (adding .h to the name) */
+static int touch_file(const char *file)
+{
+	struct stat sb;
+	int fd;
+	char *d;
+	char name[128];
+
+	strcpy(name, file);
+	strcat(name, ".h");
+
+	/* Open existing file */
+	fd = open(name, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+	if (fd == -1) {
+		if (errno != ENOENT)
+			return 1;
+		/*
+		 * Create directory components,
+		 * unless they exist already.
+		 */
+		d = name;
+		while ((d = strchr(d, '/'))) {
+			*d = 0;
+			if (stat(name, &sb) && mkdir(name, 0755))
+				return 1;
+			*d++ = '/';
+		}
+		/* Directories created, now create file. */
+		fd = open(name, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+		if (fd == -1)
+			return 1;
+	}
+	close(fd);
+	return 0;
+}
+
 int conf_split_config(void)
 {
 	char *name, path[128];
 	char *s, *d, c;
 	struct symbol *sym;
-	struct stat sb;
-	int res, i, fd;
+	int res, i;
 
 	name = getenv("KCONFIG_AUTOCONFIG");
 	if (!name)
@@ -601,45 +636,17 @@ int conf_split_config(void)
 		 *	different from 'no').
 		 */
 
-		/* Replace all '_' and append ".h" */
+		/* Replace all '_' with '/' */
 		s = sym->name;
 		d = path;
 		while ((c = *s++)) {
 			c = tolower(c);
 			*d++ = (c == '_') ? '/' : c;
 		}
-		strcpy(d, ".h");
-
-		/* Assume directory path already exists. */
-		fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
-		if (fd == -1) {
-			if (errno != ENOENT) {
-				res = 1;
-				break;
-			}
-			/*
-			 * Create directory components,
-			 * unless they exist already.
-			 */
-			d = path;
-			while ((d = strchr(d, '/'))) {
-				*d = 0;
-				if (stat(path, &sb) && mkdir(path, 0755)) {
-					res = 1;
-					goto out;
-				}
-				*d++ = '/';
-			}
-			/* Try it again. */
-			fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
-			if (fd == -1) {
-				res = 1;
-				break;
-			}
-		}
-		close(fd);
+		*d = '\0';
+		if (touch_file(path))
+			return 1;
 	}
-out:
 	if (chdir("../.."))
 		return 1;
 
Signature · 1 line
1.5.1.rc3.gaa453
#11 lore raw

PATCH 2/2 kconfig/kbuild: fix dependency problem

In reply to Sam Ravnborg
Quoted 1 line

From bbc89026f3e5d9e437ce4cd26d3013fe226103e2 Mon Sep 17 00:00:00 2001

From: Sam Ravnborg <[email protected]>
Date: Sat, 31 Mar 2007 09:34:46 +0200
Subject: [PATCH] kconfig/kbuild: fix dependency problem

Commit 2e3646e51b2d6415549b310655df63e7e0d7a080 changed the way the split config tree is built, but failed to also adjust fixdep accordingly - if changing a config option from or to m, files referencing the respective CONFIG_..._MODULE (but not the corresponding CONFIG_...) didn't get rebuilt.

This happens because tristate symbol has three values represented by different CONFIG_ symbols:

=n => CONFIG_SYMBOL undefined
=y => CONFIG_SYMBOL equals 1
=m => CONFIG_SYMBOL_MODULE equals 1

But conf_split_config did not support the _MODULE syntax and therefore no include/config/symbol/module.h file was generated/touched when changing a symbol to/from m. Thus make did nt pick up the change and rebuild failed.

This patch teaches conf_split_config to support the _MODULE variant.

This fixes a problem reported by Randy Dunlap <[email protected]>. arch/i386/kernel/apm.o revealed this bug.
Original fix was posted by: "Jan Beulich" <[email protected]> which inspired this better fix.

---
 scripts/kconfig/confdata.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
scripts/kconfig/confdata.c +6 -0
index ff6b39b..4137961 100644
@@ -646,6 +646,12 @@ int conf_split_config(void)
 		*d = '\0';
 		if (touch_file(path))
 			return 1;
+		/* For tristate symbols we need to touch symbol/module.h too */
+		if (sym->type == S_TRISTATE) {
+			strcat(path, "/module");
+			if (touch_file(path))
+				return 1;
+		}
 	}
 	if (chdir("../.."))
 		return 1;
Signature · 1 line
1.5.1.rc3.gaa453
#12 lore raw
In reply to Sam Ravnborg

Hi,

Quoted 11 lines · Sam Ravnborg

The problem is that tristate symbol represent three values. =n => CONFIG_SYMBOL is undefined
=y => CONFIG_SYMBOL is defined
=m => COMFIG_SYMBOL_MODULE is defined

The function split_config does not take into account the different values and 'fixing' this in fixdep is wrong. Because fixdep does not know if the variable is a tristate symbol or not so it can either blindly remove _MODULE (your patch)
or each time it encounters _MODULE check for a symbol with and without _MODULE.

What really matters is that CONFIG_SYMBOL changed, one could optimize for the COMFIG_SYMBOL_MODULE case, but I don't think it's worth it, especially
...

Quoted 5 lines

The better fix is to teach the split_config function that for tristate symbols two files shall be created in the include/config hirachy. So for apm this gets:
include/config/apm.h
include/config/apm/module.h

if it requires thousands of new inodes for a feature which should be rarely used.

Quoted 2 lines

This will make kconfig behave correct the day that someone add a config symbol with a _MODULE suffix.

I'd rather reserve that namespace, if it allows for the simpler version to just map all symbols to the basic config symbol name.

bye, Roman

#13 lore raw
Quoted 26 lines · Roman Zippel

Hi,

Quoted 11 lines · Sam Ravnborg

The problem is that tristate symbol represent three values. =n => CONFIG_SYMBOL is undefined
=y => CONFIG_SYMBOL is defined
=m => COMFIG_SYMBOL_MODULE is defined

The function split_config does not take into account the different values and 'fixing' this in fixdep is wrong. Because fixdep does not know if the variable is a tristate symbol or not so it can either blindly remove _MODULE (your patch)
or each time it encounters _MODULE check for a symbol with and without _MODULE.

What really matters is that CONFIG_SYMBOL changed, one could optimize for the COMFIG_SYMBOL_MODULE case, but I don't think it's worth it, especially
...

Quoted 5 lines

The better fix is to teach the split_config function that for tristate symbols two files shall be created in the include/config hirachy. So for apm this gets:
include/config/apm.h
include/config/apm/module.h

Quoted 8 lines

if it requires thousands of new inodes for a feature which should be rarely used.

Quoted 2 lines

This will make kconfig behave correct the day that someone add a config symbol with a _MODULE suffix.

I'd rather reserve that namespace, if it allows for the simpler version to just map all symbols to the basic config symbol name.

OK, I have forwarded Jan's patch for inclusion (the bug-fix part).

Sam

Quoted 2 lines

bye, Roman