[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In https://reviews.llvm.org/D48782#1164786, @labath wrote:

> For the purposes of integration testing I think it would be better to just 
> silently accept these so that all tests work "out of the box" with dwz. It 
> might be nice to hide these details in a wrapper script.


FYI the LLDB testsuite uses DWZ in a weird way so that the DWZ common file 
(external file in `/usr/lib/debug/.dwz/` with DIEs shared by multiple `.debug` 
files) can be tested by LLDB testsuite. It makes a copy of the `.debug` file so 
that the DWZ-tool thinks most DIEs are used by two files and makes a DWZ common 
file for them. This is why such script has no use outside of the run by LLDB 
testsuite. This is why the script would not be useful on its own. Also 
currently the DWZ is ran always only on one file (two files - the one file 
being duplicated) while in practice one runs DWZ on multiple similar files (all 
files of a rpm package being built) - which is never done by the LLDB testsuite 
(the file duplication tests the DWZ common files more thoroughly IMO).


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil requested changes to this revision.
jankratochvil added inline comments.
This revision now requires changes to proceed.



Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:238
 
+ifneq (,$(wildcard $(DWP)))
+  MAKE_DWP=YES

I was thinking DWP should be specifiable by user but `make check-lldb 
DWP=/usr/bin/dwp` has no effect. And `DWP` is not set by any `build*(` 
functions in `packages/Python/lldbsuite/test/plugins/builder_base.py`, sorry 
but I do not find that obvious.



Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:528
+ifeq "$(MAKE_DWP)" "YES"
+   $(DWP) -e "$(EXE)" -o "$(EXE).dwp"
+   rm -f $(OBJECTS:.o=.dwo)

The patch as exported by `Download Raw Diff` says:
```../../../make/Makefile.rules:529: *** missing separator.  Stop.
```
(after forcing it by `DWP=/usr/bin/dwp` in that `Makefile.rules` file)


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I suppose we can add an off-by-default DWP mode so that it can be used for 
integration testing.

However, I wouldn't consider any future DWP changes "tested" if the code is 
only exercised via this mode. As I said above, I think the majority of DWP code 
can be exercised  without even running a process via `lldb-test`. If there are 
any odd corner casees that can be only reached by running the full pipeline, 
then we add a couple of specialized tests which explicitly set MAKE_DWP=YES, 
and are run unconditionally (similar to how we have two debug_names tests run 
via dotest because the functionality was not accessible via lldb-test).

After some thought, I don't think the script idea I came up with initially is 
necessary here, though the situation may be different for DWZ, as the dwz 
utility seems to be quite temperamental -- IIRC it will error out if the input 
file happens to not contain a .debug_info section. For the purposes of 
integration testing I think it would be better to just silently accept these so 
that all tests work "out of the box" with dwz. It might be nice to hide these 
details in a wrapper script.


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Puyan Lotfi via Phabricator via lldb-commits]

plotfi added a comment.



> What's the medium-to-long term solution?

Havn't fully fleshed that out yet.




Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:520
 $(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
$(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o 
"$(EXE)"
 else

jankratochvil wrote:
> Shouldn't be $(LLVM_DWP) even here? DWZ mode has its command even here.
Ah, I can add it. I wasn't sure if dwp was something that works with dylibs or 
not. Thought there was something like dsym for those. 



Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:546
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DSYM)" "$(EXE)" "$(EXE)"
 endif
 endif

jankratochvil wrote:
> Shouldn't be $(LLVM_DWP) even here? DWZ mode has its command even here.
I'm not sure about this one. 


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Puyan Lotfi via Phabricator via lldb-commits]

plotfi updated this revision to Diff 155452.

https://reviews.llvm.org/D48782

Files:
  packages/Python/lldbsuite/test/make/Makefile.rules


Index: packages/Python/lldbsuite/test/make/Makefile.rules
===
--- packages/Python/lldbsuite/test/make/Makefile.rules
+++ packages/Python/lldbsuite/test/make/Makefile.rules
@@ -235,6 +235,22 @@
CFLAGS_NO_DEBUG = -O0 $(ARCHFLAG)$(ARCH) $(FRAMEWORK_INCLUDES) 
$(ARCH_CFLAGS) $(CFLAGS_EXTRAS)
 endif
 
+ifneq (,$(wildcard $(DWP)))
+  MAKE_DWP=YES
+  VALID_DWP_BIN=YES
+endif
+
+ifeq "$(MAKE_DWP)" "YES"
+  MAKE_DWO=YES
+  ifndef DWP
+$(error dwp not found, please set DWP.)
+  endif
+  ifneq "$(VALID_DWP_BIN)" "YES"
+$(error Invalid dwp, please set DWP to a path that exists.)
+  endif
+endif
+
+
 ifeq "$(MAKE_DWO)" "YES"
CFLAGS += -gsplit-dwarf
 endif
@@ -508,6 +524,10 @@
 else
 $(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
$(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+ifeq "$(MAKE_DWP)" "YES"
+   $(DWP) -e "$(EXE)" -o "$(EXE).dwp"
+   rm -f $(OBJECTS:.o=.dwo)
+endif
 endif
 
 #--


Index: packages/Python/lldbsuite/test/make/Makefile.rules
===
--- packages/Python/lldbsuite/test/make/Makefile.rules
+++ packages/Python/lldbsuite/test/make/Makefile.rules
@@ -235,6 +235,22 @@
CFLAGS_NO_DEBUG = -O0 $(ARCHFLAG)$(ARCH) $(FRAMEWORK_INCLUDES) $(ARCH_CFLAGS) $(CFLAGS_EXTRAS)
 endif
 
+ifneq (,$(wildcard $(DWP)))
+  MAKE_DWP=YES
+  VALID_DWP_BIN=YES
+endif
+
+ifeq "$(MAKE_DWP)" "YES"
+  MAKE_DWO=YES
+  ifndef DWP
+$(error dwp not found, please set DWP.)
+  endif
+  ifneq "$(VALID_DWP_BIN)" "YES"
+$(error Invalid dwp, please set DWP to a path that exists.)
+  endif
+endif
+
+
 ifeq "$(MAKE_DWO)" "YES"
CFLAGS += -gsplit-dwarf
 endif
@@ -508,6 +524,10 @@
 else
 $(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
$(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+ifeq "$(MAKE_DWP)" "YES"
+   $(DWP) -e "$(EXE)" -o "$(EXE).dwp"
+   rm -f $(OBJECTS:.o=.dwo)
+endif
 endif
 
 #--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.



Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:520
 $(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
$(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o 
"$(EXE)"
 else

Shouldn't be $(LLVM_DWP) even here? DWZ mode has its command even here.



Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:546
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DSYM)" "$(EXE)" "$(EXE)"
 endif
 endif

Shouldn't be $(LLVM_DWP) even here? DWZ mode has its command even here.


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

The DWZ-testmode Makefile.rules part. 

The DWZ-mode is written as another test matrix dimension.  Compared to this 
DWP-mode it autodetects if all the tools are available on the host for that 
testsuite mode.
I agree that some `EXPENSIVE_CHECKS` would be appropriate for both DWZ and DWP 
as general LLVM/LLDB development should not break only specifically these modes.
I do not think it matters whether there is an external wrapper script or 
whether the commands are specified as a Makefile rule/macro, sure I will adapt 
it if anyone requests an external command.


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: jankratochvil.
labath added a comment.

In https://reviews.llvm.org/D48782#1149051, @plotfi wrote:

> In https://reviews.llvm.org/D48782#1148498, @alexshap wrote:
>
> > @labath
> >
> > > I am not denying that there is value in running the dotest suite in all 
> > > of these modes. In fact, I think that (the fact that we can use the same 
> > > tests to exercise a lot of different scenarios) is one of the strengths 
> > > ?>of our test suite. However, I don't believe all of these modes should 
> > > be inflicted onto everyone running lldb tests or be our first and only 
> > > line of defense against regressions.
> >
> > for what it's worth - not sure how much you care about my opinion, but i 
> > think it's an important point but it doesn't actually contradict or prevent 
> > your second point regarding adding regression tests using lldb-test, 
> > however i think those should be added over time (sadly no tests were added 
> > when the support for .dwp was implemented / introduced) (not in this patch).
> >  I think that the approach of this patch is still useful, this mode can be 
> > off by default, but if smb needs to run all the tests with dwps - it's easy 
> > to do by passing or setting a variable (for example).
>
>
> yes, thats the near term solution.


What's the medium-to-long term solution?

> In https://reviews.llvm.org/D48782#1148929, @JDevlieghere wrote:
> 
>> In https://reviews.llvm.org/D48782#1148376, @labath wrote:
>>
>> > Then, for the integration test part, I propose to come up with a more 
>> > generic way to specify the kind of debug info to generate. I don't have 
>> > this fully thought out yet, but I have been thinking of something that 
>> > could wrap the compiler invocation with some user specified script. Then 
>> > we could use the same mechanism to run DWP and DWZ with any kind of crazy 
>> > compiler flags we think of (which is definitely useful when bringing up 
>> > support for a new kind of debug info format). If someone has a particular 
>> > interest in a specific combo, he can set up a bot which runs tests in this 
>> > mode (details about who would be responsible for fixing failures TBD)
>>
>>
>> I really like that idea. It sounds similar to the EXPENSIVE_CHECKS we have 
>> for LLVM. Personally I'd love to have an overnight job that runs the test 
>> suite with DWARF5 for example.
> 
> 
> Oh, I see. Have some generic option for debug format to make this problem 
> simpler every time it arises.

The thing is, we *already* have multiple needs for that. Maybe you could work 
with @jankratochvil to come up with a solution that would work for both DWP and 
DWZ? It sounds to me like both of these formats (and even dSYM) could be 
described by "there is some tool which runs after we link the main 
executable/library". If we could have a generic way to specify this as an 
argument to test suite, then we could solve both problems together. As the 
tools are unlikely to take the same arguments (and IIRC, for DWZ there are 
multiple commands we need to issue), we can have wrapper scripts (checked into 
the utils folder or somewhere), which hides these differences.

Does that make sense?


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Puyan Lotfi via Phabricator via lldb-commits]

plotfi added a comment.

In https://reviews.llvm.org/D48782#1148498, @alexshap wrote:

> @labath
>
> > I am not denying that there is value in running the dotest suite in all of 
> > these modes. In fact, I think that (the fact that we can use the same tests 
> > to exercise a lot of different scenarios) is one of the strengths ?>of our 
> > test suite. However, I don't believe all of these modes should be inflicted 
> > onto everyone running lldb tests or be our first and only line of defense 
> > against regressions.
>
> for what it's worth - not sure how much you care about my opinion, but i 
> think it's an important point but it doesn't actually contradict or prevent 
> your second point regarding adding regression tests using lldb-test, however 
> i think those should be added over time (sadly no tests were added when the 
> support for .dwp was implemented / introduced) (not in this patch).
>  I think that the approach of this patch is still useful, this mode can be 
> off by default, but if smb needs to run all the tests with dwps - it's easy 
> to do by passing or setting a variable (for example).


yes, thats the near term solution.

In https://reviews.llvm.org/D48782#1148288, @aprantl wrote:

> Is your plan to add dwp as another dimension in the test matrix (an equal 
> citizen of DWARF, dSYM, DWO) or something that would be on or off for an 
> entire run of the suite, or something only exercised by few specialized 
> testcases?


another dimension, off by default

In https://reviews.llvm.org/D48782#1148929, @JDevlieghere wrote:

> In https://reviews.llvm.org/D48782#1148376, @labath wrote:
>
> > Then, for the integration test part, I propose to come up with a more 
> > generic way to specify the kind of debug info to generate. I don't have 
> > this fully thought out yet, but I have been thinking of something that 
> > could wrap the compiler invocation with some user specified script. Then we 
> > could use the same mechanism to run DWP and DWZ with any kind of crazy 
> > compiler flags we think of (which is definitely useful when bringing up 
> > support for a new kind of debug info format). If someone has a particular 
> > interest in a specific combo, he can set up a bot which runs tests in this 
> > mode (details about who would be responsible for fixing failures TBD)
>
>
> I really like that idea. It sounds similar to the EXPENSIVE_CHECKS we have 
> for LLVM. Personally I'd love to have an overnight job that runs the test 
> suite with DWARF5 for example.


Oh, I see. Have some generic option for debug format to make this problem 
simpler every time it arises.




Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:238
 
+ifneq (,$(wildcard $(LLVM_DWP)))
+  MAKE_DWP=YES

alexshap wrote:
> aprantl wrote:
> > Is the fact this this is *llvm-*dwp critical, or are llvm-dwp and GNU dwp 
> > interchangeable? In the latter case, I'd prefer to drop the LLVM part from 
> > the variable.
> llvm-dwp and dwp should both work
Ah good. I will do that. 


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D48782#1148376, @labath wrote:

> Then, for the integration test part, I propose to come up with a more generic 
> way to specify the kind of debug info to generate. I don't have this fully 
> thought out yet, but I have been thinking of something that could wrap the 
> compiler invocation with some user specified script. Then we could use the 
> same mechanism to run DWP and DWZ with any kind of crazy compiler flags we 
> think of (which is definitely useful when bringing up support for a new kind 
> of debug info format). If someone has a particular interest in a specific 
> combo, he can set up a bot which runs tests in this mode (details about who 
> would be responsible for fixing failures TBD)


I really like that idea. It sounds similar to the EXPENSIVE_CHECKS we have for 
LLVM. Personally I'd love to have an overnight job that runs the test suite 
with DWARF5 for example.


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap added a comment.

@labath

> I am not denying that there is value in running the dotest suite in all of 
> these modes. In fact, I think that (the fact that we can use the same tests 
> to exercise a lot of different scenarios) is one of the strengths ?>of our 
> test suite. However, I don't believe all of these modes should be inflicted 
> onto everyone running lldb tests or be our first and only line of defense 
> against regressions.

for what it's worth - not sure how much you care about my opinion, but i think 
it's an important point but it doesn't actually contradict or prevent your 
second point regarding adding regression tests using lldb-test, however i think 
those should be added over time (sadly no tests were added when the support for 
.dwp was implemented / introduced) (not in this patch).
I think that the approach of this patch is still useful, this mode can be off 
by default, but if smb needs to run all the tests with dwps - it's easy to do 
by passing or setting a variable (for example).


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap added inline comments.



Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:238
 
+ifneq (,$(wildcard $(LLVM_DWP)))
+  MAKE_DWP=YES

aprantl wrote:
> Is the fact this this is *llvm-*dwp critical, or are llvm-dwp and GNU dwp 
> interchangeable? In the latter case, I'd prefer to drop the LLVM part from 
> the variable.
llvm-dwp and dwp should both work


https://reviews.llvm.org/D48782



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits