labath added inline comments.
Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3496
+
+ auto Decompressor = llvm::object::Decompressor::create(
+ section->GetName().GetStringRef(),
tzik wrote:
> This adds new dependency to LLVM Object
tzik added inline comments.
Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3496
+
+ auto Decompressor = llvm::object::Decompressor::create(
+ section->GetName().GetStringRef(),
This adds new dependency to LLVM Object component.
Could
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL320813: ObjectFileELF: Add support for compressed sections
(authored by labath, committed by ).
Changed prior to commit:
clayborg accepted this revision.
clayborg added a comment.
Move #include of "llvm/Object/Decompressor.h" into CPP file and this is good to
go.
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.h:24
#include "lldb/lldb-private.h"
+#include "llvm/Object/Decompressor.h"
labath updated this revision to Diff 126975.
labath added a comment.
The version where Section::GetFileSize reports the on-disk (compressed) size. I
also like the idea of renaming Section::GetByteSize to something more
descriptive, and I'll make a follow-up patch to do that.
https://reviews.llvm
labath added a comment.
In https://reviews.llvm.org/D40616#952432, @clayborg wrote:
> Does that sound right?
Yes, I'll get on it.
https://reviews.llvm.org/D40616
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-
clayborg added a comment.
Sounds good,. So the solution will be:
- Section::GetFileSize() will return the size in bytes of the section data as
it appears in the file
- Section::GetByteSize() will return the size in bytes for when this section is
loaded into process memory (we might consider ren
labath added a comment.
In https://reviews.llvm.org/D40616#951324, @clayborg wrote:
> I think GetFileSize() should remain the number of bytes of the section on
> disk and we should add new API if we need to figure out the decompressed
> size. Or maybe when we get bytes from a compressed section
davide added a comment.
In https://reviews.llvm.org/D40616#951256, @labath wrote:
> @davide: Any thoughts on `.yaml` as a test file suffix?
I think this is fine.
https://reviews.llvm.org/D40616
___
lldb-commits mailing list
lldb-commits@lists.llv
> On Dec 11, 2017, at 10:25 AM, Zachary Turner wrote:
>
> What about adding GetMemorySize?
We already have that as Section::GetByteSize().
> On Mon, Dec 11, 2017 at 10:23 AM Greg Clayton via Phabricator
> mailto:revi...@reviews.llvm.org>> wrote:
> clayborg added a comment.
>
> I think GetFi
What about adding GetMemorySize?
On Mon, Dec 11, 2017 at 10:23 AM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:
> clayborg added a comment.
>
> I think GetFileSize() should remain the number of bytes of the section on
> disk and we should add new API if we need to figure out the
clayborg added a comment.
I think GetFileSize() should remain the number of bytes of the section on disk
and we should add new API if we need to figure out the decompressed size. Or
maybe when we get bytes from a compressed section we are expected to always
just get the raw bytes, then we check
labath added a comment.
@davide: Any thoughts on `.yaml` as a test file suffix?
@clayborg: What do you think about my comment about GetFileSize() of compressed
sections
In https://reviews.llvm.org/D40616#940408, @labath wrote:
> This rewrites the test in terms on the new lldb-test utility. It
zturner added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:20
+ - Name:.bogus
+# CHECK-NOT: .bogus
+Type:SHT_PROGBITS
labath wrote:
> zturner wrote:
> > labath wrote:
> > > zturner wrote:
> > > > You should probab
labath added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:20
+ - Name:.bogus
+# CHECK-NOT: .bogus
+Type:SHT_PROGBITS
zturner wrote:
> labath wrote:
> > zturner wrote:
> > > You should probably put this as the very
zturner added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:1
+# REQUIRES: zlib
+# RUN: yaml2obj %s > %t
labath wrote:
> zturner wrote:
> > labath wrote:
> > > It's right here. (I'm open to suggestions where to place it).
> > I see. I think p
zturner added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:20
+ - Name:.bogus
+# CHECK-NOT: .bogus
+Type:SHT_PROGBITS
labath wrote:
> zturner wrote:
> > You should probably put this as the very first check stateme
labath updated this revision to Diff 125110.
labath added a comment.
Rebase on master and update the test.
https://reviews.llvm.org/D40616
Files:
lit/CMakeLists.txt
lit/Modules/compressed-sections.yaml
lit/Modules/lit.local.cfg
lit/lit.cfg
lit/lit.site.cfg.in
source/Plugins/ObjectFi
labath marked 2 inline comments as done.
labath added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:1
+# REQUIRES: zlib
+# RUN: yaml2obj %s > %t
zturner wrote:
> labath wrote:
> > It's right here. (I'm open to suggestions where to place it).
>
zturner added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:1
+# REQUIRES: zlib
+# RUN: yaml2obj %s > %t
labath wrote:
> It's right here. (I'm open to suggestions where to place it).
I see. I think part of the reason I didn't notice it is bec
labath added inline comments.
Comment at: lit/Modules/compressed-sections.yaml:1
+# REQUIRES: zlib
+# RUN: yaml2obj %s > %t
It's right here. (I'm open to suggestions where to place it).
https://reviews.llvm.org/D40616
Did you forget to add the new test to the changeset?
On Thu, Nov 30, 2017 at 6:19 AM Pavel Labath wrote:
> I have to say I quite like how this test turned out to look like. In
> terms of the last night's SBAPI vs. lldb-mi vs. whateever discussion,
> i'd can say that there is nothing preventing th
I have to say I quite like how this test turned out to look like. In
terms of the last night's SBAPI vs. lldb-mi vs. whateever discussion,
i'd can say that there is nothing preventing this test written in
terms of the SB API. I didn't do it that way, because I have
automatically written the test at
labath updated this revision to Diff 124926.
labath added a comment.
This rewrites the test in terms on the new lldb-test utility. It should be
applied on top of https://reviews.llvm.org/D40636.
While doing that, I noticed a discrepancy in the data presented by the object
file interface -- for G
And of course, one such discipline would be "only use SB API's"...
Jim
> On Nov 29, 2017, at 5:00 PM, Jim Ingham wrote:
>
> I thought we weren't talking about why you want this lldb-test, but rather
> why tests that poke deep into the lldb_private API's are or are not
> appropriate for your
I thought we weren't talking about why you want this lldb-test, but rather why
tests that poke deep into the lldb_private API's are or are not appropriate for
your test dingus.
The thing I worry about is if you start using this for very special purpose
"reach deep into the lldb_private API's"
Many reasons.
1) a test that looks like this:
```
RUN: yaml2obj %p/Inputs/compressed-section.yaml > %t/compressed-section.obj
RUN: lldb-test %s | FileCheck %s
create-module -f %t/compressed-section.obj
dump-module-sections compressed-section.obj
; CHECK: Section: .hello_elf
; CHECK-NEXT: Size:
> On Nov 29, 2017, at 3:46 PM, Zachary Turner wrote:
>
> FWIW, it can certainly use the SB API where it makes sense, but I think
> requiring that it only use the SB API would be very limiting and a big
> mistake.
>
> The entire point of a tool such as this is that it allows you to dig deep
FWIW, it can certainly use the SB API where it makes sense, but I think
requiring that it only use the SB API would be very limiting and a big
mistake.
The entire point of a tool such as this is that it allows you to dig deep
into internals that would be difficult to access otherwise.
On Wed, Nov
> On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits
> wrote:
>
>
>
> On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote:
> I'm mostly basing this concern on the bad effect this had on gdb all of whose
> testing was expect based command scraping. gdb is a tool that's much closer
On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote:
> I'm mostly basing this concern on the bad effect this had on gdb all of
> whose testing was expect based command scraping. gdb is a tool that's much
> closer to lldb than any of the compiler tools so that experience seems
> relevant. It's been
> On Nov 29, 2017, at 2:13 PM, Greg Clayton wrote:
>
>
>> On Nov 29, 2017, at 2:10 PM, Jason Molenda via lldb-commits
>> wrote:
>>
>> The last time this discussion came up (Sep 2016), I pointed out that a great
>> approach here would be to use the lldb-mi driver. The MI (Machine
>> Inter
> On Nov 29, 2017, at 2:10 PM, Jason Molenda via lldb-commits
> wrote:
>
> The last time this discussion came up (Sep 2016), I pointed out that a great
> approach here would be to use the lldb-mi driver. The MI (Machine Interface)
> is a JSON-like debugger UI which closely models how people
The last time this discussion came up (Sep 2016), I pointed out that a great
approach here would be to use the lldb-mi driver. The MI (Machine Interface)
is a JSON-like debugger UI which closely models how people use the lldb command
line driver. It was written at Cygnus (long before JSON, hen
I'm mostly basing this concern on the bad effect this had on gdb all of whose
testing was expect based command scraping. gdb is a tool that's much closer to
lldb than any of the compiler tools so that experience seems relevant. It's
been a decade or so since I worked on gdb, but back when I wa
On Wed, Nov 29, 2017 at 1:38 PM, Jim Ingham wrote:
> I'm a little confused by your response.
>
> My stated objection to command output dependent tests is and has always been
> that they make the test dependent on the details of command output. Over
> time doing so makes it hard to modify comman
I'm a little confused by your response.
My stated objection to command output dependent tests is and has always been
that they make the test dependent on the details of command output. Over time
doing so makes it hard to modify command output. This is sort of related to
interactivity, in th
We’ve had this discussion several times, but at the end of the day nothing
has really changed with the larger llvm project having adopted this model
and having spent significant effort in moving forward with it.
Normally, the reason we don't use this model for LLDB tests is because they
require in
jingham added a comment.
It does look a little weird as a unit test, but to me this is mostly because it
would been much simpler to write it as a regular SB API test.
Anyway, I really don't want the details of the text output of lldb commands to
become API. Our experience with gdb was that ove
davide added a comment.
Another very good reason for writing a `FileCheck` test rather than a unittest
is that writing unittest is tedious :)
In particular for new contributors, FileCheck tests are much easier to write
and in this case testing the API surface doesn't seem to add much value.
ht
davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.
This one is a little weird when written as unittest. Not the worst thing, but I
agree it should use `llvm-lit`.
Can you give it a shot, Pavel? If that doesn't work, we should at least
zturner added a comment.
Note that there's no interactivity here. This is "feed some input, get some
output, make sure the output is correct". That's exactly what FileCheck is
designed for. This isn't even testing the public API, it's testing the private
API. We should prefer testing the ac
zturner added a comment.
For the same reason that the entire rest of the LLVM project and all other
subprojects do it, when it makes sense and the nature of the test lends itself
to it.
https://reviews.llvm.org/D40616
___
lldb-commits mailing list
clayborg added a comment.
Why would we text scrape when we can test the API?
https://reviews.llvm.org/D40616
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner added a comment.
It's too bad this has to be written as a unit test, because this would be the
perfect candidate for a FileCheck style test.
Probably a long shot, but have you tried the llvm-lit project? Last time I
tried it, it basically worked, but there were only a handful of tests
labath created this revision.
Herald added subscribers: aprantl, mgorny, emaste.
We use the llvm decompressor to decompress SHF_COMPRESSED sections. This enables
us to read data from debug info sections, which are sometimes compressed,
particuarly in the split-dwarf case. This functionality is on
46 matches
Mail list logo
