[PATCH v4 1/2] BZ #17645, fix slow DSO sorting behavior in dynamic loader -- Testing infrastructure

[Florian Weimer]

* Chung-Lin Tang:

> diff --git a/elf/Makefile b/elf/Makefile
> index 543800f4be..ff5dd7edab 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -446,6 +446,33 @@ tests-special += $(objpfx)order-cmp.out $(objpfx)tst-array1-cmp.out \
>  		 $(objpfx)tst-unused-dep-cmp.out
>  endif
>  
> +# DSO sorting tests:
> +# The dso-ordering-test.py script generates testcase source files in $(objpfx),
> +# creating a $(objpfx)<testcase-name>-dir for each testcase, and creates a
> +# Makefile fragment to be included.
> +define include_dsosort_tests
> +$(objpfx)$(1).tmp-makefile: $(1)
> +	$(PYTHON) $(..)scripts/dso-ordering-test.py \
> +	--description-file $$< --objpfx $(objpfx) --output-makefile $$@
> +include $(objpfx)$(1).tmp-makefile
> +endef

Maybe use a different suffix?  .mk?

The file isn't really temporary, so .tmp-makefile seems misleading.

> +# Generate from each testcase description file
> +$(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
> +$(eval $(call include_dsosort_tests,dso-sort-tests-2.def))

So the issue here is that each .def file can produce multiple targets,
hence it's not possible to use a pattern rule.

We already have one prior use of $(eval …), so I guess this is okay.
(We used to have a ban on it.)

> +# This function can be used to define a single DSO sorting test, completely
> +# here in the Makefile, for example:
> +#   $(eval $(call single_dsosort_test,testcase-name,'a->b->c','c>b>a>{}<a<b<c'))
> +# Currently all tests are defined in description files, and this function is
> +# not utilized, but kept here for possible conveniences.
> +define single_dsosort_test
> +$(objpfx)$(1).tmp-makefile:
> +	$(PYTHON) $(..)scripts/dso-ordering-test.py --objpfx $(objpfx) \
> +	$(2) $(1) $(3) --output-makefile $$@
> +include $(objpfx)$(1).tmp-makefile
> +endef

I suggest not to include this because it's unused.

> diff --git a/elf/dso-sort-tests-1.def b/elf/dso-sort-tests-1.def
> new file mode 100644
> index 0000000000..51337ec3c7
> --- /dev/null
> +++ b/elf/dso-sort-tests-1.def
> @@ -0,0 +1,61 @@
> +# DSO sorting test descriptions.
> +# This file is to be processed by ../scripts/dso-ordering-test.py, see usage
> +# in elf/Makefile for how it is executed.

As far as I can tell, this is a reasonable collection of tests.

What's missing is sorting behavior if the main program has a soname.
This seems relevant given that the actual patch seems to change behavior
there.  Currently, the expectation is that if the main program has a
soname, it is always initialized last.  It does not participate in the
dependency sort.  This can change with new algorithm and a DT_NEEDED on
the main program.

Also not included is a symbol interposition test.  I think this could be
achieved with a per-DSO marker that triggers the definition of a global,
exported function.  This function would then print the DSO name or main
program name into the output.  Some care needs to be taken (by the test
author) to define the function only in some of the DSOs, to get the
desired interposition behavior.  But that I think it is not critical to
have this functionality included from the start.

> +# We test both dynamic loader sorting algorithms
> +tunable_option: glibc.rtld.dynamic_sort=1
> +tunable_option: glibc.rtld.dynamic_sort=2

Given that these tunables do not exist yet, should they be added with
the second patch?  This will also avoid the XFAIL.

> diff --git a/scripts/dso-ordering-test.py b/scripts/dso-ordering-test.py
> new file mode 100644
> index 0000000000..7f0515312c
> --- /dev/null
> +++ b/scripts/dso-ordering-test.py

Would you please reformat to a line length of 79?  And we generally do
not follow the space-before-paren rule for Python source code, e.g.,
“exit(1)”, not “exit (1)”.

> +The '!' operator after object names turns on permutation of its
> +dependencies, e.g. while a->[bcd] only generates one set of objects,
> +with 'a.so' built with a link line of "b.so c.so d.so", for a!->[bcd]
> +permutations of a's dependencies creates multiple testcases with
> +different link line orders: "b.so c.so d.so", "c.so b.so d.so",
> +"b.so d.so c.so", etc. Note that for a <test-name> specified on
> +the script command-line, multiple <test-name_1>, <test-name_2>, etc.
> +tests will be generated (e.g. for a!->[bc]!->[de], eight tests with
> +different link orders for a, b, and c will be generated)

Is it worh noting that it's necessary to specify the ordering of the
permuted DSOs separately, so that a unique output is generated?

> +# Lexer for tokens
> +tokenspec = [ ("OBJ",      r"([0-9a-zA-Z]+)"),
> +              ("DEP",      r"->"),
> +              ("CALLREF",  r"=>"),
> +              ("OBJSET",   r"\[([0-9a-zA-Z]+)\]"),

Should 0-9 be dropped here, given that the DSO names must be single
letters?

> +# Main line parser of description language
> +def parse_description_string (t, descr_str):

> +        else:
> +            print ("Error: unknown token '%s'" % (value))
> +            exit (-1)

Would it be possible to include line number and character offset
information here?  I think it would help the test author to fix syntax
errors.

> +def process_testcase (t):
> +    global objpfx
> +    assert t.test_name
> +
> +    base_test_name = t.test_name

Should the .def file name be included in the test name?  This avoid
accidental collisions between different .def files.

> +    if "#" in t.deps:
> +        deps = t.deps["#"]
> +        if '*' in deps:
> +            t.deps["#"].remove ('*')
> +            t.add_deps (["#"], non_dep_tgt_objs)

As far as I understand it, '#' is a synthetic marker.  Would it make
sense to use a global variable for it, or put some comment somewhere to
explain its purpose?

> +                        # A circular dependency is satisfied by making a fake DSO
> +                        # tagged with the correct SONAME
> +                        depstr = (" $(objpfx)" + test_subdir + "/"
> +                                  + test_name + "-" + dep + ".FAKE.so")

I think elsewhere we call them “linkmods”.  But perhaps that's just me.

> +    makefile.write \
> +    ("$(objpfx)%s.out: $(objpfx)%s/%s.sh%s $(common-objpfx)support/test-run-command\n"
> +     % (t.test_name, test_subdir, t.test_name,
> +        expected_output_files))
> +    makefile.write ("\t$(SHELL) $< $(common-objpfx) '$(test-wrapper-env)' "
> +                    "'$(run-program-env)' > $@; $(evaluate-test)\n")

I've already mentioned that test-run-command does not link on some
targets.  I tried to fix it up, but I couldn't get it to build even on
some Linux targets (link failures related to unwinding).  I think we
should revisit the timeout handling separately.  Or perhaps depend on
the timeout tool from coreutils.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill