On Thu, Jul 23, 2020 at 12:23 PM Andrei Ziureaev
<andrei.ziureaev@xxxxxxx> wrote:
>
>
> On 7/23/20 3:46 PM, Rob Herring wrote:
> > On Tue, Jul 21, 2020 at 9:59 AM Andrei Ziureaev <andrei.ziureaev@xxxxxxx> wrote:
> >> Add support for building and loading plugins.
> >>
> >> A plugin interface makes it possible to add checks in any language. It
> >> also allows these checks to print dts source line information.
> >>
> >> Signed-off-by: Andrei Ziureaev <andrei.ziureaev@xxxxxxx>
> >> ---
> >> Makefile | 32 +++++++++++-
> >> dtc-plugin.h | 27 ++++++++++
> >> dtc.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> dtc.h | 47 +++++++++++++++++
> >> 4 files changed, 243 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index cb256e8..76bba53 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -25,6 +25,8 @@ WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
> >> -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> >> CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
> >>
> >> +LDLIBS_dtc += -ldl -export-dynamic
> >> +
> >> BISON = bison
> >> LEX = flex
> >> SWIG = swig
> >> @@ -65,14 +67,17 @@ ifeq ($(HOSTOS),darwin)
> >> SHAREDLIB_EXT = dylib
> >> SHAREDLIB_CFLAGS = -fPIC
> >> SHAREDLIB_LDFLAGS = -fPIC -dynamiclib -Wl,-install_name -Wl,
> >> +PLUGIN_ldflags = -fPIC -dynamiclib
> >> else ifeq ($(HOSTOS),$(filter $(HOSTOS),msys cygwin))
> >> SHAREDLIB_EXT = so
> >> SHAREDLIB_CFLAGS =
> >> SHAREDLIB_LDFLAGS = -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
> >> +PLUGIN_ldflags = -shared
> >> else
> >> SHAREDLIB_EXT = so
> >> SHAREDLIB_CFLAGS = -fPIC
> >> SHAREDLIB_LDFLAGS = -fPIC -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
> >> +PLUGIN_ldflags = -fPIC -shared
> >> endif
> >>
> >> #
> >> @@ -186,7 +191,32 @@ ifneq ($(MAKECMDGOALS),libfdt)
> >> endif
> >> endif
> >>
> >> +#
> >> +# Rules for plugins
> >> +#
> >> +PLUGIN_dir = plugins
> >> +PLUGIN_subdirs = $(patsubst %/,%,$(filter-out $(PLUGIN_dir)/,$(dir $(wildcard $(PLUGIN_dir)/*/))))
> >> +PLUGIN_names = $(notdir $(PLUGIN_subdirs))
> >> +PLUGIN_libs = $(join $(PLUGIN_subdirs),$(patsubst %,/%.$(SHAREDLIB_EXT),$(PLUGIN_names)))
> >> +PLUGIN_CLEANFILES += $(PLUGIN_dir)/*.$(SHAREDLIB_EXT)
> >> +PLUGIN_CLEANFILES += $(addprefix $(PLUGIN_dir)/*/,*.o *.$(SHAREDLIB_EXT))
> >> +
> >> +include $(wildcard $(PLUGIN_dir)/*/Makefile.*)
> >> +
> >> +plugins: $(PLUGIN_libs)
> >> +
> >> +$(PLUGIN_dir)/%.$(SHAREDLIB_EXT): $(PLUGIN_dir)/%.o
> >> + @$(VECHO) LD $@
> >> + $(LINK.c) $(PLUGIN_ldflags) -o $@ $< $(PLUGIN_LDLIBS_$(notdir $*))
> >> + ln -sf $(patsubst $(PLUGIN_dir)/%,%,$@) $(PLUGIN_dir)/$(notdir $@)
> >> +
> >> +$(PLUGIN_dir)/%.o: $(PLUGIN_dir)/%.c
> >> + @$(VECHO) CC $@
> >> + $(CC) $(CPPFLAGS) $(CFLAGS) $(PLUGIN_CFLAGS_$(notdir $*)) -o $@ -c $<
> >>
> >> +plugins_clean:
> >> + @$(VECHO) CLEAN "(plugins)"
> >> + rm -f $(PLUGIN_CLEANFILES)
> >>
> >> #
> >> # Rules for libfdt
> >> @@ -330,7 +360,7 @@ endif
> >> STD_CLEANFILES = *~ *.o *.$(SHAREDLIB_EXT) *.d *.a *.i *.s core a.out vgcore.* \
> >> *.tab.[ch] *.lex.c *.output
> >>
> >> -clean: libfdt_clean pylibfdt_clean tests_clean
> >> +clean: libfdt_clean pylibfdt_clean tests_clean plugins_clean
> >> @$(VECHO) CLEAN
> >> rm -f $(STD_CLEANFILES)
> >> rm -f $(VERSION_FILE)
> >> diff --git a/dtc-plugin.h b/dtc-plugin.h
> >> index 35c3d19..9ec6b81 100644
> >> --- a/dtc-plugin.h
> >> +++ b/dtc-plugin.h
> >> @@ -10,6 +10,8 @@
> >> #include <stdint.h>
> >> #include <stdbool.h>
> >>
> >> +#include "srcpos.h"
> >> +
> > This belongs in patch 3.
> >
> >> struct dt_info {
> >> const char *version;
> > Okay, so we don't need this as you are checking the version in a different way.
>
>
> Oops, I forgot to remove this.
>
> >
> >> unsigned int dtsflags;
> >> @@ -170,4 +172,29 @@ static inline size_t type_marker_length(struct marker *m)
> >> return 0;
> >> }
> >>
> >> +struct plugin_arg {
> >> + char *key; /* A non-empty string */
> >> + char *value; /* NULL or a non-empty string */
> >> +};
> >> +
> >> +struct plugin_version {
> >> + int major; /* Incompatible changes */
> >> + int minor; /* Somewhat incompatible changes */
> > A minor bump should be compatible changes. A new field added to the
> > end of a struct for example.
> >
> >> +};
> >> +
> >> +#define DTC_PLUGIN_API_VERSION ((struct plugin_version){ .major = 0, .minor = 0 })
> >> +
> >> +static inline bool dtc_plugin_default_version_check(struct plugin_version v)
> > Maybe 'dtc_ver' instead of 'v'.
> >
> >> +{
> >> + struct plugin_version this_v = DTC_PLUGIN_API_VERSION;
> >> + return v.major == this_v.major && v.minor == this_v.minor;
> > I think, by default, minor version differences should be okay.
>
> I originally wanted this whole interface to be similar to gcc's (it
> turned out nothing like gcc's), and gcc's default version check is the
> strictest check possible. I guess the idea is that plugin authors can
> write their own less strict checks if they want. But if minor versions
> are compatible, then maybe there's no point in checking them, even by
> default.
Okay. Let's see if there's any other opinions.
> >> +}
> >> +
> >> +/* Hook definitions */
> >> +typedef int (*init_fn_t)(struct plugin_version v, int argc, struct plugin_arg *argv);
> >> +typedef void (*validate_fn_t)(struct dt_info *dti);
> >> +
> >> +#define TYPE_TO_NAME(type) dtc_plugin_v0_##type
> >> +#define EXPORT_FUNCTION(type, fn) type TYPE_TO_NAME(type) = (fn)
> >> +
> >> #endif /* DTC_PLUGIN_H */
> >> diff --git a/dtc.c b/dtc.c
> >> index bdb3f59..5a9b118 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -4,6 +4,7 @@
> >> */
> >>
> >> #include <sys/stat.h>
> >> +#include <dlfcn.h>
> >>
> >> #include "dtc.h"
> >> #include "srcpos.h"
> >> @@ -47,7 +48,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >>
> >> /* Usage related data. */
> >> static const char usage_synopsis[] = "dtc [options] <input file>";
> >> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AThv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:l:L:R:S:p:a:fb:i:H:sW:E:@AThv";
> >> static struct option const usage_long_opts[] = {
> >> {"quiet", no_argument, NULL, 'q'},
> >> {"in-format", a_argument, NULL, 'I'},
> >> @@ -55,6 +56,8 @@ static struct option const usage_long_opts[] = {
> >> {"out-format", a_argument, NULL, 'O'},
> >> {"out-version", a_argument, NULL, 'V'},
> >> {"out-dependency", a_argument, NULL, 'd'},
> >> + {"plugin", a_argument, NULL, 'l'},
> >> + {"plugin-dir", a_argument, NULL, 'L'},
> >> {"reserve", a_argument, NULL, 'R'},
> >> {"space", a_argument, NULL, 'S'},
> >> {"pad", a_argument, NULL, 'p'},
> >> @@ -89,6 +92,13 @@ static const char * const usage_opts_help[] = {
> >> "\t\tasm - assembler source",
> >> "\n\tBlob version to produce, defaults to "stringify(DEFAULT_FDT_VERSION)" (for dtb and asm output)",
> >> "\n\tOutput dependency file",
> >> + "\n\tLoad a plugin and, optionally, pass it an argument.\n"
> >> + "\tUsage: -l <plugin name>[,key][,value]\n"
> >> + "\t\tplugin name - the name of the shared object without the extension (can contain a path)\n"
> >> + "\t\tkey - the name of the argument\n"
> >> + "\t\tvalue - can be omitted\n"
> >> + "\tExample: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]",
> > Should be '-L':
> >
> > -L path/to/another-plugin
>
>
> "-L" specifies the directory and "-l" - a plugin name with an optional
> path and an argument. I guess we can simplify this, get rid of "-l", and
> just have "-L" take a full path and an argument. Since this is going to
> be called mostly from makefiles, the extra typing shouldn't be a
> problem. We could also pass all the arguments at once with something like:
>
> -L path/plugin-name,"key=value key2=value2"
Ah, I was confused thinking that 'path/to/another-plugin' was a path.
I'd just drop 'path/to/' here.
I think a separate search path is better. We could still do all args
together. I don't really have a preference on that. Probably would
want a comma separator rather than a space though.
Rob
