MaskRay wrote:
> I like this idea a lot, but I have some reservations about the implementation.
>
> For one, I think this patch is too big. I once heard someone say "if you have
> bullet points in your patch description, then the patch is doing too much".
> While I don't think we should go as far as to create a separate PR for each
> of your bullet points, I do believe that splitting it up into a couple of
> pieces would go along way towards making it easier to review.
>
> I also see that some of the functionality is guarded by `IsInMemory()`, which
> doesn't sounds like the right thing to do, as the we should in principle be
> able to use the same code for parsing parsing section-header-less object
> files on disk. These aren't exactly common, but like @MaskRay said, section
> headers aren't strictly needed for linked object files, and it's much easier
> to make test using these.
>
> Finally, I think that structuring some of this code as "fallback" is not
> ideal, as it can cause some data can be parsed twice (I think it happens at
> least with ELF notes in this patch). Even if that's innocuous , I don't think
> it's right because the two mechanisms (program and section headers) are just
> different ways of finding the same data. I think it'd be cleaner if this was
> implemented as a two-step process:
>
> 1. find the data (e.g., notes): This will look into section and program
> headers to find the appropriate bytes (I might even argue it should look at
> program headers first, as that's what the operating system will use)
> 2. use the data (regardless of where it comes from)
>
> I realise this feedback isn't very specific, but that's because I found it
> very hard to follow everything that's going on in this patch. I'm sure I'll
> be able to be more specific on the partial patches (and maybe some of my
> assumptions will turn out to be incorrect). As a first patch in the series,
> I'd recommend teaching lldb to parse section-header-less object files. Right
> now, if I run lldb on such a file, it will tell me that it's empty (has zero
> sections). Making the program headers visible would lay the foundation for
> other changes, and it would also be the smallest testable piece of
> functionality (by dumping the section list).
>
> @MaskRay can you recommend a good to create these kinds of files? I was
> thinking of a combination `yaml2obj` + `llvm-objcopy --strip-sections`
> (because IIRC yaml2obj always inserts some sections into the output), but
> maybe there's something better (?)
yaml2obj omits the section header table when `NoHeaders: true` is specified.
```
- Type: SectionHeaderTable
NoHeaders: true
```
However, obj2yaml doesn't create `NoHeaders: true` yet.
If the test utilizes `ld.lld`, `llvm-objcopy --strip-sections` will be needed.
GNU ld 2.41 supports `-z nosectionheader`, which lld doesn't support yet (if
there is enough interest I can add it).
https://github.com/llvm/llvm-project/pull/100900
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
