labath 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 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 (?) 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
