On 21-11-18 02:03, Ian Lance Taylor wrote: > On Wed, Nov 14, 2018 at 6:45 AM, Tom de Vries <tdevr...@suse.de> wrote: >> On 14-11-18 14:25, Jakub Jelinek wrote: >>> On Wed, Nov 14, 2018 at 02:08:05PM +0100, Tom de Vries wrote: >>>>> +btest_dwz_CFLAGS = $(AM_CFLAGS) -g -O0 >>>> >>>> Hmm, I already discovered that specifying the -O0 doesn't work, since >>>> it's overridden by $(CFLAGS). >>>> >>>> With a hack like this: >>>> ... >>>> diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am >>>> index 2fec9bbb4b6..8bdf13b3546 100644 >>>> --- a/libbacktrace/Makefile.am >>>> +++ b/libbacktrace/Makefile.am >>>> @@ -99,11 +99,14 @@ check_PROGRAMS += btest >>>> if HAVE_DWZ >>>> >>>> btest_dwz_SOURCES = btest_dwz.c testlib.c >>>> -btest_dwz_CFLAGS = $(AM_CFLAGS) -g -O0 >>>> +btest_dwz_CFLAGS = $(AM_CFLAGS) -g >>>> btest_dwz_LDADD = libbacktrace.la >>>> >>>> check_PROGRAMS += btest_dwz >>>> >>>> +btest_dwz-btest_dwz.o: btest_dwz.c >>>> + $(AM_V_CC)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) >>>> $(AM_CPPFLAGS) $(CPPFLAGS) $(btest_dwz_CFLAGS) $(CFLAGS) -O0 -c -o >>>> btest_dwz-btest_dwz.o `test -f 'btest_dwz.c' || echo >>>> '$(srcdir)/'`btest_dwz.c >>> >>> Can't you instead do something like: >>> btest_dwz.o: CFLAGS += -g -O0 >>> or something similar >> >> Hi, >> >> yes, that works, thanks. >> >>> (whatever the corresponding goal is)? >> >> The goal is to run the testcase with a setting lower than -O2, such that >> we can successfully run a substantial portion of the test without >> needing support for DW_FORM_GNU_ref_alt. >> >> [ At O2 we get constprop versions of some functions, which have an >> abstract origin, which tends to be moved to the common debug file by dwz >> -m, after which we need support for DW_FORM_GNU_ref_alt to get to the >> name of the function. ] >> >>> Otherwise, the patch looks generally ok to me, >> >> Great. >> >>> but yes, I've been wondering >>> how you can get away with DW_FORM_GNU_ref_alt not implemented properly. >>> >> >> Indeed, DW_FORM_GNU_ref_alt support is required to make this work in >> general. >> >> But I observed that implementing just DW_FORM_GNU_strp_alt improves on >> the current situation, so I thought it was worthwhile submitting this as >> a separate patch. >> >> Updated patch attached (which also rewrites btest_dwz.c to an include of >> btest.c, while disabling the inline tests that require DW_FORM_GNU_ref_alt). > > Unfortunately the tests don't pass for me. > > rm -f btest_dwz.debug > cp btest_dwz btest_dwz_2 > cp btest_dwz btest_dwz_3 > dwz -m btest_dwz.debug btest_dwz_2 btest_dwz_3 > FAIL: btest_dwz_2 > FAIL: btest_dwz_3 > >> libbacktrace/btest_dwz_2 > test1: [0]: missing file name or function name > FAIL: backtrace_full noinline > test3: [0]: missing file name or function name > FAIL: backtrace_simple noinline > PASS: backtrace_syminfo variable > >> libbacktrace/btest_dwz_3 > test1: [0]: missing file name or function name > FAIL: backtrace_full noinline > test3: [0]: missing file name or function name > FAIL: backtrace_simple noinline > PASS: backtrace_syminfo variable >
Hmm, I can't reproduce that. I'm reworking this patch into a patch series that includes also support for DW_FORM_GNU_ref_alt, so I'm hoping that that will fix the failures you're seeing. >> +#define INLINE_TESTS 0 >> +#include "btest.c" > > Please avoid this kind of #include game. If you need to skip some > tests (why?) use a command line option. If you need to compile with > different options, use automake features. > The patch series with DW_FORM_GNU_ref_alt support added no longer requires this. >> +elf_open_debugfile_by_debugaltlink (struct backtrace_state *state, > > Do we need this function? It seems to be the same as > elf_find_debugfile_by_debuglink. Hmm, that's right. I've now updated the patch in my patch series. I'll resubmit once the fix for PR88063 is in trunk (I need the keeping track of units that that patch adds, for DW_FORM_GNU_ref_alt support). Thanks for the review, - Tom
