Hi Florian, thanks for the review. Response to various points below, and updated patch attached. On 2021/1/12 4:51 AM, Florian Weimer wrote: > * Chung-Lin Tang: >> +# DSO sorting tests: >> +# The dso-ordering-test.py script generates testcase source files in $(objpfx), >> +# creating a $(objpfx)-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. I've changed it to ".generated-makefile" About the use of $(eval ...), I didn't find another way to allow wrapping the generation + include of the makefile fragment into a function, so glad that eval is not completely banned... >> +# 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>{}> +# 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. Okay, removed. >> 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. Hmm, I guess there is indeed some concern here. I'll think more about this before responding to your review of the algorithm parts. OTOH, does the ELF specification dictate in a "hard" sense that the main program is always at the start of the resulting sort, or is it unspecified? > 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. That is kind of starting to wade out of the DSO sorting behavior testing goal that this tool was originally designed to do, I think, but could be interesting to expand to. >> +# 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. I've tried to make the two patches not textually dependent on one another. I also think they'll probably be approved together, so I'll leave this as is right now. >> 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)”. Done. > >> +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 specified on >> +the script command-line, multiple , , 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? I am not entirely sure what you mean here? Do you mean to warn "be careful that all permutations do have the same single expected output"? >> +# 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? The script now allows longer strings for DSO names now, e.g. see the test case in elf/dso-sort-tests-2.def. >> +# 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. I've added line number reporting, but character column information is a bit too complicated to implement quickly (at least with my current Python-fu), so please bear with that. >> +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. We place all files related to a testcase inside $(common-objpfx)/elf/-dir/ subdirectory, the .def file name isn't important at this level. >> + 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? I've added some related comments around the 'process_main_program' routine. >> + # 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. So I'll leave it like that for now. This is something internal that can be changed any time anyways. >> + 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. Probably the most important change in this v5 update: I've discovered that linking elf/static-stubs.o is enough for test-run-command to link and work; in fact it seems we don't even need any thread library here. I've added a support/Depend file with "elf" in it. The attached version is v5 of the Testing infrastructure parts, with the above mentioned changes. I'll respond to the Algorithm review in a day or two. Thanks, Chung-Lin