[PATCH] framework for building modules externally

[PATCH] framework for building modules externally

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]


Hi,

Attached patch makes it possible to build modules externally, by:

  - Installing headers in system include dir.

  - Exporting two ABI tags (a build-time macro and a run-time variable)
    for run-time comparison.

  - Exporting a makefile with COMMON_*FLAGS variables.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: external_modules.diff --]
[-- Type: text/x-diff, Size: 2750 bytes --]

2008-11-01  Robert Millan  <rmh@aybabtu.com>

	* Makefile.in (include_DATA): New variable.
	(build_flags.mk): New target.
	(pkgdata_DATA): Add `build_flags.mk'.
	(install-local): Install $(include_DATA) files in $(includedir).

	* include/grub/misc.h (GRUB_ABI): New macro (initialize as 0).
	(grub_abi): New variable prototype.
	* kern/main.c (grub_abi): New variable (initialize as GRUB_ABI).

Index: Makefile.in
===================================================================
--- Makefile.in	(revision 1889)
+++ Makefile.in	(working copy)
@@ -152,6 +152,13 @@ ifeq (, $(UNIFONT_HEX))
 else
 pkgdata_DATA += unicode.pff ascii.pff
 
+include_DATA = $(shell find $(srcdir)/include -name \*.h | sed -e 's,^$(srcdir)/,,g') \
+			$(srcdir)/include/grub/cpu
+
+pkgdata_DATA += build_flags.mk
+build_flags.mk:
+	echo -en "COMMON_ASFLAGS=$(COMMON_ASFLAGS)\nCOMMON_CFLAGS=$(COMMON_CFLAGS)\nCOMMON_LDFLAGS=$(COMMON_LDFLAGS)\n" > $@
+
 # Arrows and lines are needed to draw the menu, so we always include them
 UNICODE_ARROWS=0x2190-0x2193
 UNICODE_LINES=0x2501-0x251B
@@ -226,6 +233,19 @@ install-local: all
 	  dest="`echo $$file | sed 's,.*/,,'`"; \
 	  $(INSTALL_DATA) $$dir$$file $(DESTDIR)$(libdir)/grub/$$dest; \
 	done
+	$(mkinstalldirs) $(DESTDIR)$(includedir)
+	@list='$(include_DATA)'; \
+	for file in $$list; do \
+	  if test -f "$$file"; then dir=; else dir="$(srcdir)/"; fi; \
+	  dest="`echo $$file | sed 's,include/,,'`"; \
+	  destdir="`echo $$dest | sed 's,\(^\|/\)[^/]*$$,,g'`"; \
+	  $(mkinstalldirs) $(DESTDIR)$(includedir)/$$destdir; \
+	  if test -f "$$dir$$file"; then \
+	    $(INSTALL_DATA) $$dir$$file $(DESTDIR)$(includedir)/$$dest; \
+	  elif test -L "$$dir$$file"; then \
+	    cp -d $$dir$$file $(DESTDIR)$(includedir)/$$dest; \
+	  fi; \
+	done
 
 install-strip:
 	$(MAKE) INSTALL_PROGRAM="$(INSTALL_STRIP_PROGRAM)" install
Index: kern/main.c
===================================================================
--- kern/main.c	(revision 1889)
+++ kern/main.c	(working copy)
@@ -28,6 +28,8 @@
 #include <grub/env.h>
 #include <grub/mm.h>
 
+grub_uint32_t grub_abi = GRUB_ABI;
+
 void
 grub_module_iterate (int (*hook) (struct grub_module_header *header))
 {
Index: include/grub/misc.h
===================================================================
--- include/grub/misc.h	(revision 1889)
+++ include/grub/misc.h	(working copy)
@@ -25,6 +25,10 @@
 #include <grub/symbol.h>
 #include <grub/err.h>
 
+/* Increase this number every time GRUB ABI is changed.  */
+#define GRUB_ABI	0
+extern grub_uint32_t EXPORT_VAR(grub_abi);
+
 #define ALIGN_UP(addr, align) (((grub_uint64_t)addr + align - 1) & ~(align - 1))
 
 #define grub_dprintf(condition, fmt, args...) grub_real_dprintf(__FILE__, __LINE__, condition, fmt, ## args);

Re: [PATCH] framework for building modules externally

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

On Sat, Nov 01, 2008 at 01:32:29PM +0100, Robert Millan wrote:
> 
> Hi,
> 
> Attached patch makes it possible to build modules externally, by:
> 
>   - Installing headers in system include dir.
> 
>   - Exporting two ABI tags (a build-time macro and a run-time variable)
>     for run-time comparison.
> 
>   - Exporting a makefile with COMMON_*FLAGS variables.

Looks like I missed some important details.  The headers and some macros are
not enough, as we need to reproduce part of the build system to build a module
externally.  This new patch provides the following:

  In pkgdata:	genmodsrc.sh genmk.rb
  In pkglib:	config.h grub_script.tab.h build_env.mk
  In includedir:	header files

How to use:

cat > Makefile.in << EOF
srcdir = /usr/share/grub
builddir = /usr/lib/grub/i386-pc
include $(builddir)/build_env.mk

pkglib_MODULES += mymod.mod
mymod_mod_SOURCES = mymod.c
mymod_mod_CFLAGS = $(COMMON_CFLAGS)
mymod_mod_LDFLAGS = $(COMMON_LDFLAGS)
EOF
ruby /usr/share/grub/genmk.rb < Makefile.in > Makefile
make

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: external_modules.diff --]
[-- Type: text/x-diff, Size: 3517 bytes --]

2008-11-01  Robert Millan  <rmh@aybabtu.com>

	* Makefile.in (PKGLIB): Add $(pkglib_BUILDDIR).
	(PKGDATA): Add $(pkgdata_SRCDIR).
	(pkglib_BUILDDIR): New variable.
	(pkgdata_SRCDIR): New variable.
	(build_env.mk): New target.
	(include_DATA): New variable.
	(install-local): Install $(include_DATA) files in $(includedir).

	* include/grub/misc.h (GRUB_ABI): New macro (initialize as 0).
	(grub_abi): New variable prototype.
	* kern/main.c (grub_abi): New variable (initialize as GRUB_ABI).

Index: Makefile.in
===================================================================
--- Makefile.in	(revision 1891)
+++ Makefile.in	(working copy)
@@ -105,8 +105,8 @@
 MKFILES = $(patsubst %.rmk,%.mk,$(RMKFILES))
 
 PKGLIB = $(pkglib_IMAGES) $(pkglib_MODULES) $(pkglib_PROGRAMS) \
-	$(pkglib_DATA) $(lib_DATA)
-PKGDATA = $(pkgdata_DATA)
+	$(pkglib_DATA) $(lib_DATA) $(pkglib_BUILDDIR)
+PKGDATA = $(pkgdata_DATA) $(pkgdata_SRCDIR)
 PROGRAMS = $(bin_UTILITIES) $(sbin_UTILITIES)
 SCRIPTS = $(bin_SCRIPTS) $(sbin_SCRIPTS) $(grub-mkconfig_SCRIPTS)
 
@@ -163,6 +163,22 @@
 	ruby $(srcdir)/util/unifont2pff.rb 0x0-0x7f $(UNICODE_ARROWS) $(UNICODE_LINES) $(UNIFONT_HEX) > $@
 endif
 
+# Used for building modules externally
+pkglib_BUILDDIR += build_env.mk
+build_env.mk: Makefile
+	(\
+	echo "TARGET_CC=$(TARGET_CC)" ; \
+	echo "TARGET_CFLAGS=$(TARGET_CFLAGS)" ; \
+	echo "TARGET_CPPFLAGS=$(TARGET_CPPFLAGS) -I$(pkglibdir)" ; \
+	echo "STRIP=$(STRIP)" ; \
+	echo "COMMON_ASFLAGS=$(COMMON_ASFLAGS)" ; \
+	echo "COMMON_CFLAGS=$(COMMON_CFLAGS)" ; \
+	echo "COMMON_LDFLAGS=$(COMMON_LDFLAGS)"\
+	) > $@
+pkglib_BUILDDIR += config.h grub_script.tab.h
+pkgdata_SRCDIR += genmodsrc.sh genmk.rb
+include_DATA += $(shell find include -name \*.h) include/grub/cpu
+
 all-local: $(PROGRAMS) $(PKGLIB) $(PKGDATA) $(SCRIPTS) $(MKFILES)
 
 install: install-local
@@ -175,6 +191,19 @@
 	  dest="`echo $$file | sed 's,.*/,,'`"; \
 	  $(INSTALL_DATA) $$dir$$file $(DESTDIR)$(pkglibdir)/$$dest; \
 	done
+	$(mkinstalldirs) $(DESTDIR)$(includedir)
+	@list='$(include_DATA)'; \
+	for file in $$list; do \
+	  if test -f "$$file"; then dir=; else dir="$(srcdir)/"; fi; \
+	  dest="`echo $$file | sed 's,include/,,'`"; \
+	  destdir="`echo $$dest | sed 's,\(^\|/\)[^/]*$$,,g'`"; \
+	  $(mkinstalldirs) $(DESTDIR)$(includedir)/$$destdir; \
+	  if test -f "$$dir$$file"; then \
+	    $(INSTALL_DATA) $$dir$$file $(DESTDIR)$(includedir)/$$dest; \
+	  elif test -L "$$dir$$file"; then \
+	    cp -d $$dir$$file $(DESTDIR)$(includedir)/$$dest; \
+	  fi; \
+	done
 	$(mkinstalldirs) $(DESTDIR)$(pkgdatadir)
 	@list='$(PKGDATA)'; \
 	for file in $$list; do \
Index: kern/main.c
===================================================================
--- kern/main.c	(revision 1891)
+++ kern/main.c	(working copy)
@@ -28,6 +28,8 @@
 #include <grub/env.h>
 #include <grub/mm.h>
 
+grub_uint32_t grub_abi = GRUB_ABI;
+
 void
 grub_module_iterate (int (*hook) (struct grub_module_header *header))
 {
Index: include/grub/misc.h
===================================================================
--- include/grub/misc.h	(revision 1891)
+++ include/grub/misc.h	(working copy)
@@ -25,6 +25,10 @@
 #include <grub/symbol.h>
 #include <grub/err.h>
 
+/* Increase this number every time GRUB ABI is changed.  */
+#define GRUB_ABI	0
+extern grub_uint32_t EXPORT_VAR(grub_abi);
+
 #define ALIGN_UP(addr, align) (((grub_uint64_t)addr + align - 1) & ~(align - 1))
 
 #define grub_dprintf(condition, fmt, args...) grub_real_dprintf(__FILE__, __LINE__, condition, fmt, ## args);

Re: [PATCH] framework for building modules externally

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Sat, Nov 01, 2008 at 01:32:29PM +0100, Robert Millan wrote:
>> Hi,
>>
>> Attached patch makes it possible to build modules externally, by:
>>
>>   - Installing headers in system include dir.
>>
>>   - Exporting two ABI tags (a build-time macro and a run-time variable)
>>     for run-time comparison.
>>
>>   - Exporting a makefile with COMMON_*FLAGS variables.
> 
> Looks like I missed some important details.  The headers and some macros are
> not enough, as we need to reproduce part of the build system to build a module
> externally.  This new patch provides the following:

Interesting idea. However how do you define GRUB ABI :) ? What is it,
kernel, certain set of modules, or how :) ?

Versioning of the modules would be a good idea indeed to make sure
mismatched modules are not tried together.

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Tue, Nov 04, 2008 at 08:55:52PM +0200, Vesa Jääskeläinen wrote:
> Robert Millan wrote:
> > On Sat, Nov 01, 2008 at 01:32:29PM +0100, Robert Millan wrote:
> >> Hi,
> >>
> >> Attached patch makes it possible to build modules externally, by:
> >>
> >>   - Installing headers in system include dir.
> >>
> >>   - Exporting two ABI tags (a build-time macro and a run-time variable)
> >>     for run-time comparison.
> >>
> >>   - Exporting a makefile with COMMON_*FLAGS variables.
> > 
> > Looks like I missed some important details.  The headers and some macros are
> > not enough, as we need to reproduce part of the build system to build a module
> > externally.  This new patch provides the following:
> 
> Interesting idea. However how do you define GRUB ABI :) ? What is it,
> kernel, certain set of modules, or how :) ?

Anything that can possibly break an external module.  I.e. any interface
exported by either the kernel or any of the modules.

(I expect we'd bump this number a lot)

> Versioning of the modules would be a good idea indeed to make sure
> mismatched modules are not tried together.

This should never happen if user always runs grub-install (which he should
do anyway unless he's looking for trouble).  My idea for external modules is
that a separate package can put them in $pkglibdir and then grub-install will
automaticaly include them the next time it's called.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Tue, Nov 04, 2008 at 08:55:52PM +0200, Vesa Jääskeläinen wrote:
>> Robert Millan wrote:
>>> On Sat, Nov 01, 2008 at 01:32:29PM +0100, Robert Millan wrote:
>>>> Hi,
>>>>
>>>> Attached patch makes it possible to build modules externally, by:
>>>>
>>>>   - Installing headers in system include dir.
>>>>
>>>>   - Exporting two ABI tags (a build-time macro and a run-time variable)
>>>>     for run-time comparison.
>>>>
>>>>   - Exporting a makefile with COMMON_*FLAGS variables.
>>> Looks like I missed some important details.  The headers and some macros are
>>> not enough, as we need to reproduce part of the build system to build a module
>>> externally.  This new patch provides the following:
>> Interesting idea. However how do you define GRUB ABI :) ? What is it,
>> kernel, certain set of modules, or how :) ?
> 
> Anything that can possibly break an external module.  I.e. any interface
> exported by either the kernel or any of the modules.
> 
> (I expect we'd bump this number a lot)

Did you have some plan how to automate this ABI versioning :) ? If it is
not automated I can almost guarantee that we get it wrong at one point
of time :)

>> Versioning of the modules would be a good idea indeed to make sure
>> mismatched modules are not tried together.
> 
> This should never happen if user always runs grub-install (which he should
> do anyway unless he's looking for trouble).  My idea for external modules is
> that a separate package can put them in $pkglibdir and then grub-install will
> automaticaly include them the next time it's called.

How do you treat differences in ABI ?

Dis-allow loading of module with different value of ABI ? Or were you
planning that module itself adapts to different versions of GRUB 2 ABI's?

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Tue, Nov 04, 2008 at 09:13:43PM +0200, Vesa Jääskeläinen wrote:
> 
> Did you have some plan how to automate this ABI versioning :) ? If it is
> not automated I can almost guarantee that we get it wrong at one point
> of time :)

It's not a big problem, at least not for my intended use of this feature.
Debian packages can use strong versioned dependencies to prevent
breakage.  Also, I wouldn't mind reviewing the ChangeLog for "missed" ABI
bumps before I upload a package.

And it surely would have been bumped several times between official releases.

> >> Versioning of the modules would be a good idea indeed to make sure
> >> mismatched modules are not tried together.
> > 
> > This should never happen if user always runs grub-install (which he should
> > do anyway unless he's looking for trouble).  My idea for external modules is
> > that a separate package can put them in $pkglibdir and then grub-install will
> > automaticaly include them the next time it's called.
> 
> How do you treat differences in ABI ?
> 
> Dis-allow loading of module with different value of ABI ? Or were you
> planning that module itself adapts to different versions of GRUB 2 ABI's?

The module itself could:

GRUB_MOD_INIT(foo)
{
  if (grub_abi != GRUB_ABI)
    {
      grub_printf ("abi mismatch!\n");
      return;
    }

  /* register our commands/terminals/disks/whatever */
}

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Christian Franke]

Robert Millan wrote:
>> ...
>> How do you treat differences in ABI ?
>>
>> Dis-allow loading of module with different value of ABI ? Or were you
>> planning that module itself adapts to different versions of GRUB 2 ABI's?
>>     
>
> The module itself could:
>
> GRUB_MOD_INIT(foo)
> {
>   if (grub_abi != GRUB_ABI)
>     {
>       grub_printf ("abi mismatch!\n");
>       return;
>     }
>
>   /* register our commands/terminals/disks/whatever */
> }
>
>   

Alternative: Export a symbol describing the ABI version in kernel 
("grub_abi_3_14"). Access this symbol in each module (this can be hidden 
in GRUB_MOD_INIT).

Then loading a module with wrong ABI version fails due to undefined symbol.

-- 
Christian Franke

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Wed, Nov 05, 2008 at 07:57:38AM +0100, Christian Franke wrote:
> 
> Alternative: Export a symbol describing the ABI version in kernel 
> ("grub_abi_3_14").

That requires more bytes than a 32-bit integer.

> Access this symbol in each module (this can be hidden 
> in GRUB_MOD_INIT).

And this adds code in every module, but for non-external modules we already
have reassurance that their abi is consistent (users should never bypass
grub-install;  if they do, it's likely going to break for them anyway).

I understand there's a minor benefit for programmers of external modules,
but both things are at the expense of extra size to kernel and core.img
modules.  External modules will only provide non-essential functionality, so
it's not a problem they have to check the ABI IMHO.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]


Hi,

Since it looks like the ABI part inspires more need for debate/discussion,
I propose merging the rest first.  Support for ABI tracking is not essential
for building external modules (in Debian, it's barely necessary).

Here's a new patch.  If nobody objects, I'll commit it in a few days.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: external_modules_noabi.diff --]
[-- Type: text/x-diff, Size: 2450 bytes --]

2008-11-01  Robert Millan  <rmh@aybabtu.com>

	* Makefile.in (PKGLIB): Add $(pkglib_BUILDDIR).
	(PKGDATA): Add $(pkgdata_SRCDIR).
	(pkglib_BUILDDIR): New variable.
	(pkgdata_SRCDIR): New variable.
	(build_env.mk): New target.
	(include_DATA): New variable.
	(install-local): Install $(include_DATA) files in $(includedir).

Index: Makefile.in
===================================================================
--- Makefile.in	(revision 1891)
+++ Makefile.in	(working copy)
@@ -105,8 +105,8 @@
 MKFILES = $(patsubst %.rmk,%.mk,$(RMKFILES))
 
 PKGLIB = $(pkglib_IMAGES) $(pkglib_MODULES) $(pkglib_PROGRAMS) \
-	$(pkglib_DATA) $(lib_DATA)
-PKGDATA = $(pkgdata_DATA)
+	$(pkglib_DATA) $(lib_DATA) $(pkglib_BUILDDIR)
+PKGDATA = $(pkgdata_DATA) $(pkgdata_SRCDIR)
 PROGRAMS = $(bin_UTILITIES) $(sbin_UTILITIES)
 SCRIPTS = $(bin_SCRIPTS) $(sbin_SCRIPTS) $(grub-mkconfig_SCRIPTS)
 
@@ -163,6 +163,22 @@
 	ruby $(srcdir)/util/unifont2pff.rb 0x0-0x7f $(UNICODE_ARROWS) $(UNICODE_LINES) $(UNIFONT_HEX) > $@
 endif
 
+# Used for building modules externally
+pkglib_BUILDDIR += build_env.mk
+build_env.mk: Makefile
+	(\
+	echo "TARGET_CC=$(TARGET_CC)" ; \
+	echo "TARGET_CFLAGS=$(TARGET_CFLAGS)" ; \
+	echo "TARGET_CPPFLAGS=$(TARGET_CPPFLAGS) -I$(pkglibdir)" ; \
+	echo "STRIP=$(STRIP)" ; \
+	echo "COMMON_ASFLAGS=$(COMMON_ASFLAGS)" ; \
+	echo "COMMON_CFLAGS=$(COMMON_CFLAGS)" ; \
+	echo "COMMON_LDFLAGS=$(COMMON_LDFLAGS)"\
+	) > $@
+pkglib_BUILDDIR += config.h grub_script.tab.h
+pkgdata_SRCDIR += genmodsrc.sh genmk.rb
+include_DATA += $(shell find include -name \*.h) include/grub/cpu
+
 all-local: $(PROGRAMS) $(PKGLIB) $(PKGDATA) $(SCRIPTS) $(MKFILES)
 
 install: install-local
@@ -175,6 +191,19 @@
 	  dest="`echo $$file | sed 's,.*/,,'`"; \
 	  $(INSTALL_DATA) $$dir$$file $(DESTDIR)$(pkglibdir)/$$dest; \
 	done
+	$(mkinstalldirs) $(DESTDIR)$(includedir)
+	@list='$(include_DATA)'; \
+	for file in $$list; do \
+	  if test -f "$$file"; then dir=; else dir="$(srcdir)/"; fi; \
+	  dest="`echo $$file | sed 's,include/,,'`"; \
+	  destdir="`echo $$dest | sed 's,\(^\|/\)[^/]*$$,,g'`"; \
+	  $(mkinstalldirs) $(DESTDIR)$(includedir)/$$destdir; \
+	  if test -f "$$dir$$file"; then \
+	    $(INSTALL_DATA) $$dir$$file $(DESTDIR)$(includedir)/$$dest; \
+	  elif test -L "$$dir$$file"; then \
+	    cp -d $$dir$$file $(DESTDIR)$(includedir)/$$dest; \
+	  fi; \
+	done
 	$(mkinstalldirs) $(DESTDIR)$(pkgdatadir)
 	@list='$(PKGDATA)'; \
 	for file in $$list; do \

Re: [PATCH] framework for building modules externally

[Christian Franke]

Robert Millan wrote:
> On Wed, Nov 05, 2008 at 07:57:38AM +0100, Christian Franke wrote:
>   
>> Alternative: Export a symbol describing the ABI version in kernel 
>> ("grub_abi_3_14").
>>     
>
> That requires more bytes than a 32-bit integer.
>
>   

The symbol value does not matter, the symbol name string provides enough 
space to describe any version.

GCC apparently uses a similar method ("_gcc/gxx_personality_*") to 
ensure that the correct libgcc is linked.


>> Access this symbol in each module (this can be hidden 
>> in GRUB_MOD_INIT).
>>     
>
> And this adds code in every module, but for non-external modules we already
> have reassurance that their abi is consistent (users should never bypass
> grub-install;  if they do, it's likely going to break for them anyway).
>
> I understand there's a minor benefit for programmers of external modules,
> but both things are at the expense of extra size to kernel and core.img
> modules.  External modules will only provide non-essential functionality, so
> it's not a problem they have to check the ABI IMHO.
>
>   

I agree, such a check is not necessary for the non-external modules.

But, exporting one extra "grub_abi_VERSION" symbol would only require 
one entry in the kernel symbol table and no other code.

Then, a module author has the option to check the ABI version by 
importing this symbol
(possibly by simply adding '-u grub_abi_VERSION' to ld command)


-- 
Christian Franke


PS: The current use of __FILE__ may also add extra unexpected size: For 
packaging, configure is often run outside of $srcdir with a absolute 
path name. This may result in long __FILE__ strings, like
/home/maintainer/packaging/grub/tmp/grub-1.96+20081105-1/src/grub-1.96/kern/disk.c

__FILE__ (Re: [PATCH] framework for building modules externally)

[Robert Millan]

On Wed, Nov 05, 2008 at 10:41:20PM +0100, Christian Franke wrote:
> 
> PS: The current use of __FILE__ may also add extra unexpected size: For 
> packaging, configure is often run outside of $srcdir with a absolute 
> path name. This may result in long __FILE__ strings, like
> /home/maintainer/packaging/grub/tmp/grub-1.96+20081105-1/src/grub-1.96/kern/disk.c

This has annoyed me for a while.  Do you know a proper fix?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Wed, Nov 05, 2008 at 10:41:20PM +0100, Christian Franke wrote:
> 
> Then, a module author has the option to check the ABI version by 
> importing this symbol
> (possibly by simply adding '-u grub_abi_VERSION' to ld command)

How would the whole picture look like?  Sounds like it'd mean less complexity
in external modules at the expense of more complexity in the build system.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

Robert Millan wrote:
> On Wed, Nov 05, 2008 at 10:41:20PM +0100, Christian Franke wrote:
>   
>> Then, a module author has the option to check the ABI version by 
>> importing this symbol
>> (possibly by simply adding '-u grub_abi_VERSION' to ld command)
>>     
>
> How would the whole picture look like?  Sounds like it'd mean less complexity
> in external modules at the expense of more complexity in the build system.
>
>   

Here a first proof-of-concept patch, uses hello.c as the external module.

If kernel is compiled with other GRUB_ABI_VER_SYM, insmod fails with:
"error: the symbol 'grub_abi_ver_1_96' not found"

Christian


[-- Attachment #2: grub2-abi-ver.patch --]
[-- Type: text/x-patch, Size: 1120 bytes --]

diff --git a/hello/hello.c b/hello/hello.c
index 70cbf60..4eb2483 100644
--- a/hello/hello.c
+++ b/hello/hello.c
@@ -25,6 +25,8 @@
 #include <grub/dl.h>
 #include <grub/normal.h>
 
+GRUB_ABI_VER_CHECK;
+
 static grub_err_t
 grub_cmd_hello (struct grub_arg_list *state __attribute__ ((unused)),
 		int argc __attribute__ ((unused)),
diff --git a/include/grub/dl.h b/include/grub/dl.h
index bdde089..3efc05e 100644
--- a/include/grub/dl.h
+++ b/include/grub/dl.h
@@ -93,4 +93,11 @@ void *EXPORT_FUNC(grub_dl_resolve_symbol) (const char *name);
 grub_err_t grub_arch_dl_check_header (void *ehdr);
 grub_err_t grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr);
 
+#define GRUB_ABI_VER_SYM  grub_abi_ver_1_96
+
+extern char EXPORT_VAR(GRUB_ABI_VER_SYM);
+
+#define GRUB_ABI_VER_CHECK \
+char * grub_abi_ver_check = &GRUB_ABI_VER_SYM
+
 #endif /* ! GRUB_DL_H */
diff --git a/kern/dl.c b/kern/dl.c
index b483134..32591f4 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -29,6 +29,8 @@
 #include <grub/env.h>
 #include <grub/cache.h>
 
+char GRUB_ABI_VER_SYM;
+
 #if GRUB_CPU_SIZEOF_VOID_P == 4
 
 typedef Elf32_Word Elf_Word;

Re: __FILE__ (Re: [PATCH] framework for building modules externally)

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

Robert Millan wrote:
> On Wed, Nov 05, 2008 at 10:41:20PM +0100, Christian Franke wrote:
>   
>> PS: The current use of __FILE__ may also add extra unexpected size: For 
>> packaging, configure is often run outside of $srcdir with a absolute 
>> path name. This may result in long __FILE__ strings, like
>> /home/maintainer/packaging/grub/tmp/grub-1.96+20081105-1/src/grub-1.96/kern/disk.c
>>     
>
> This has annoyed me for a while.  Do you know a proper fix?
>
>   
There is apparently no option to modify __FILE__ expansion. Therefore, 
the relative file name has to be specified in the module itself.

See attached patch for a possible fix: Each module using grub_dprintf 
(here disk.c) may specify its name in 'this_file'. When all modules are 
changed, the '#define this_file' and all #undefs can be removed.

Christian


[-- Attachment #2: grub2-dprintf-file.patch --]
[-- Type: text/x-patch, Size: 940 bytes --]

diff --git a/include/grub/misc.h b/include/grub/misc.h
index 15c18f5..36e4bcc 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -27,7 +27,9 @@
 
 #define ALIGN_UP(addr, align) (((grub_uint64_t)addr + align - 1) & ~(align - 1))
 
-#define grub_dprintf(condition, fmt, args...) grub_real_dprintf(__FILE__, __LINE__, condition, fmt, ## args);
+#define grub_dprintf(condition, fmt, args...) grub_real_dprintf(this_file, __LINE__, condition, fmt, ## args);
+#define this_file __FILE__
+
 /* XXX: If grub_memmove is too slow, we must implement grub_memcpy.  */
 #define grub_memcpy(d,s,n)	grub_memmove ((d), (s), (n))
 
diff --git a/kern/disk.c b/kern/disk.c
index ed82506..039c011 100644
--- a/kern/disk.c
+++ b/kern/disk.c
@@ -25,6 +25,9 @@
 #include <grub/time.h>
 #include <grub/file.h>
 
+#undef this_file
+static const char this_file[] = "kern/disk.c";
+
 #define	GRUB_CACHE_TIMEOUT	2
 
 /* The last time the disk was used.  */

Re: __FILE__ (Re: [PATCH] framework for building modules externally)

[Robert Millan]

On Thu, Nov 06, 2008 at 10:10:56PM +0100, Christian Franke wrote:
> 
> See attached patch for a possible fix: Each module using grub_dprintf 
> (here disk.c) may specify its name in 'this_file'. When all modules are 
> changed, the '#define this_file' and all #undefs can be removed.

This adds redundancy which later makes it more work to move code around and
rename files, etc.  Why not make it part of the build system?  I.e. to build
foo.c, you need gcc -Dfile=foo.c foo.c ?

But then again, since gcc is free software perhaps it'd make more sense to
add new candy there? (e.g. __RELATIVE_FILE__ or so).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Thu, Nov 06, 2008 at 09:43:09PM +0100, Christian Franke wrote:
> 
> Here a first proof-of-concept patch, uses hello.c as the external module.
> 
> If kernel is compiled with other GRUB_ABI_VER_SYM, insmod fails with:
> "error: the symbol 'grub_abi_ver_1_96' not found"

Ah, I thought it'd be more complex.  Seems fine to me.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Wed, Nov 05, 2008 at 10:51:23AM +0100, Robert Millan wrote:
> 
> Hi,
> 
> Since it looks like the ABI part inspires more need for debate/discussion,
> I propose merging the rest first.  Support for ABI tracking is not essential
> for building external modules (in Debian, it's barely necessary).
> 
> Here's a new patch.  If nobody objects, I'll commit it in a few days.

Committed.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Thu, Nov 06, 2008 at 09:43:09PM +0100, Christian Franke wrote:
> >
> >How would the whole picture look like?  Sounds like it'd mean less 
> >complexity
> >in external modules at the expense of more complexity in the build system.
> 
> Here a first proof-of-concept patch, uses hello.c as the external module.
> 
> If kernel is compiled with other GRUB_ABI_VER_SYM, insmod fails with:
> "error: the symbol 'grub_abi_ver_1_96' not found"

Ah, it's much simpler than I thought.

> +#define GRUB_ABI_VER_SYM  grub_abi_ver_1_96

I assume between releases we'd add a counter after the version number?  Like
grub_abi_ver_1_96_X or so?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: __FILE__ (Re: [PATCH] framework for building modules externally)

[Christian Franke]

Robert Millan wrote:
> On Thu, Nov 06, 2008 at 10:10:56PM +0100, Christian Franke wrote:
>   
>> See attached patch for a possible fix: Each module using grub_dprintf 
>> (here disk.c) may specify its name in 'this_file'. When all modules are 
>> changed, the '#define this_file' and all #undefs can be removed.
>>     
>
> This adds redundancy which later makes it more work to move code around and
> rename files, etc.  Why not make it part of the build system?  I.e. to build
> foo.c, you need gcc -Dfile=foo.c foo.c ?
>
>   

Rarely used, but makes sense.

Even leaving the source as is and using 'gcc -D__FILE__=foo.c ...' would 
work, but then gcc emits a warning.


> But then again, since gcc is free software perhaps it'd make more sense to
> add new candy there? (e.g. __RELATIVE_FILE__ or so).
>
>   
... and/or a new option '-ffile_var_relative_to=PREFIX' which removes 
PREFIX (e.g. "$srcdir/") from __FILE__ expansions. Then such a fix could 
easily be added also to other existing projects, without any sourcecode 
changes.

Christian

Re: [PATCH] framework for building modules externally

[Christian Franke]

Robert Millan wrote:
> On Thu, Nov 06, 2008 at 09:43:09PM +0100, Christian Franke wrote:
>   
>>> How would the whole picture look like?  Sounds like it'd mean less 
>>> complexity
>>> in external modules at the expense of more complexity in the build system.
>>>       
>> Here a first proof-of-concept patch, uses hello.c as the external module.
>>
>> If kernel is compiled with other GRUB_ABI_VER_SYM, insmod fails with:
>> "error: the symbol 'grub_abi_ver_1_96' not found"
>>     
>
> Ah, it's much simpler than I thought.
>
>   
>> +#define GRUB_ABI_VER_SYM  grub_abi_ver_1_96
>>     
>
> I assume between releases we'd add a counter after the version number?  Like
> grub_abi_ver_1_96_X or so?
>
>   

Yes. Symbol should be changed with each incompatible ABI change. Not 
necessary for simple ABI enhancements like new functions.

Christian

Re: [PATCH] framework for building modules externally

[phcoder]

Robert Millan wrote:
> On Wed, Nov 05, 2008 at 07:57:38AM +0100, Christian Franke wrote:
>> Alternative: Export a symbol describing the ABI version in kernel 
>> ("grub_abi_3_14").
> 
> That requires more bytes than a 32-bit integer.
You export grub_abi anyway. _3_14 is 4 bytes instead of 4 but saves. 
grub_abi_* can be a fictive variable by adding something like
       {"grub_abi_<..>", 0},
to gensymlist.sh.in

> 
>> Access this symbol in each module (this can be hidden 
>> in GRUB_MOD_INIT).
> 
> And this adds code in every module, but for non-external modules we already
> have reassurance that their abi is consistent (users should never bypass
> grub-install;  if they do, it's likely going to break for them anyway).
> 
> I understand there's a minor benefit for programmers of external modules,
> but both things are at the expense of extra size to kernel and core.img
> modules.  External modules will only provide non-essential functionality, so
> it's not a problem they have to check the ABI IMHO.
> 


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] framework for building modules externally

[Robert Millan]

On Sat, Apr 11, 2009 at 01:19:14AM +0200, phcoder wrote:
> Robert Millan wrote:
>> On Wed, Nov 05, 2008 at 07:57:38AM +0100, Christian Franke wrote:
>>> Alternative: Export a symbol describing the ABI version in kernel  
>>> ("grub_abi_3_14").
>>
>> That requires more bytes than a 32-bit integer.
> You export grub_abi anyway. _3_14 is 4 bytes instead of 4 but saves.  
> grub_abi_* can be a fictive variable by adding something like
>       {"grub_abi_<..>", 0},
> to gensymlist.sh.in

I've been thinking that this doesn't solve all the problems anyway.

*.lst files are generated in official grub tree, and it's not easy for
external packages to override that.  Perhaps we should do it at the
source level and not bother about ABI anymore... :-/

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."