[Lldb-commits] [lldb] [lldb][AIX] Added XCOFF Object File Header for AIX (PR #111814)

[Pavel Labath via lldb-commits]

labath wrote:

> > A couple of quick notes:
> > 
> > * are you sure that ObjectFileELF is the best thing to start with here? Based on the name, I would expect that ObjectFilePECOFF would be a better baseline. Or maybe the object file format is different enough from all of the existing ones that this kind of comparison is not useful?
> > * without also seeing the cpp file, it hard to judge how similar it is to the other plugin
> > * instead of redeclaring all the header types, would it be possible to use some structures from llvm? (I know that ObjectFileELF does not do that, but that's because it is very old. ObjectFilePECOFF -- a much newer class -- does just that)
> > * a lot of the object file code should be testable on its own. If you include the necessary plugin glue in this PR, then you should be able to write tests similar to those in `lldb/test/Shell/ObjectFile`. For example, if you could start with a test similar to `test/Shell/ObjectFile/ELF/basic-info.yaml` and then include enough of code in the PR to make that pass.
> 
> Hi @labath ,
> 
>     1. Actually there might be slight similarities with ObjectFilePECOFF, but the overall format structure is very different to be compared to any one of the existing formats, so I have just taken the ObjectFileELF.h only as a base to follow the general class/file arrangement, is all.
> 
>     2. As per our last discussion, I have planned to drop each of the major files separately, so I can push the .cpp file as another PR, and both of these can be used for comparison.

That's okay, though I fear you're taking this a bit too literally. I definitely wanted to split that PR up, but this is a bit too far. It would be better to have the header and the matching cpp file in the same PR, since you usually need to look at them in tandem.

That said, the main reason we (or I, at least) wanted to see this kind of a PR is to determine whether it makes sense to merge this with some of the existing code. For ObjectFileELF, I'm pretty confident the answer to that is going to be "no" (I'm slightly less sure about ObjectFilePECOFF, but I think I'd be willing to take your word for it -- I think a good litmus test would be whether the PECOFF and XCOFF parsing code in llvm shares anything). Given that, the comparison to a different object file plugin is not really interesting to me, as I believe that means this part of code should be treated as any new code that's being added to llvm. In particular, that it should be subject to the [incremental development](https://llvm.org/docs/DeveloperPolicy.html#incremental-development) policy.

This is where I was coming from when I wrote the previous message (I did not include the context, as I was in a bit of a 
hurry -- sorry). Incremental development means developing (and reviewing, and submitting) the change in small pieces that can be easily reviewed and come with their own tests. What exactly is a small piece is not easy to determine, but it's usually less than "an entire file". For an object file plugin, a relatively reasonable sequence would be:
- parse the file headers (enough so you can detect the file format and answer questions like what is its architecture)
- parse sections
- parse symbols
- other stuff (?)

I know this is going to be more work, and it's going to be a bit awkward to re-engineer a finished file into an incremental set of changes, but what you have is essentially a long-term development branch -- exactly the thing which the policy discourages. I'm not asking this because I want to be difficult, but large changes **really** are hard to review.

https://github.com/llvm/llvm-project/pull/111814