Thanks for doing this. This was something on my radar too as an odd inconsistency.

LGTM.

I'm concerned with the getHeader() call changes in ELFObjectFile, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call EF.getHeader(). Say the new function is called getFoo(): if another user then decides they want to call getFoo() on a MutableELFObject, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a MutableELFObject's point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?

LGTM, though @rupprecht should confirm too.

LGTM.

LGTM.

This goes for all the new functionality you are adding, but make sure to review the llvm-strip and llvm-objcopy documentation (llvm/docs/CommandGuide) for any changes that need to be made. For example, if any of the switches are currently listed as "ELF specific" they need to be moved into the general switch section.

In D62462#1630905, @mmpozulp wrote:

In D62462#1627011, @jhenderson wrote:

@mmpozulp, can I confirm that this is now in? It looks like Phabricator hasn't auto-closed this issue.

It's in now.

I think you need to be careful that you any methods in ELFObjectFile etc that query the header return the updated value. For example, I see there's a getStartAddress function, which returns the value of e_entry. If you change e_entry, you'll want this to return the updated value, I think.

I think some other interesting test cases are a) adding more than one symbol/section, b) adding and then removing a symbol/section, c) removing stuff and then adding stuff, showing that the iteration still works correctly.

Latest changes LGTM.

LGTM. No need to get further review from my point of view after you've fixed my remaining nit.

LGTM too.

Self note: I still need to look at coverage of the unit tests.

Addressed review comments. In particular, I have removed the pre-compiled Mach-O binaries, in favour of YAML inputs, as they're not too bad.

In D65189#1626565, @seiya wrote:

In D65189#1625303, @jhenderson wrote:

You should update your patch title and description too.

What do you think what the patch title should describe?

How about "[MC] Support returning marked up ranges in the disassembly" or "[MCInstPrinter] Add MarkupStart/MarkupEnd/WithMarkup?

@mmpozulp, can I confirm that this is now in? It looks like Phabricator hasn't auto-closed this issue.

LGTM.

I'll address the comments later today hopefully.

LGTM, with the suggestions.

The file renaming hasn't been reflected in my patch creation script for some reason. For reference:

• no-input.test was originally basic.test
• elf-berkeley.test was originally X86/elf-sizes.test
• common.test was originally X86/test-common.s

Aside from a comment change, this looks good to me. I'd wait for others to approve too though, and I wouldn't necessarily land it even then until some later patches are ready too, although I'm not sure about that.

You should update your patch title and description too.

The change looks fine to me, although I wasn't able to quickly find the list of section types anywhere to check against. Could you provide me some documentation?

Addressed review comments:

In D65991#1624866, @MaskRay wrote:

Copied from llvm/tools/llvm-objcopy/StripOpts.td

Phabricator is smart?

LGTM, with a couple of nits. I'm okay with the refactoring being left to a different change, as long as you don't leave it a long time.

LGTM.

LGTM, aside from @grimar's comment.

Not had time to look at everything in this patch, but it feels like you don't want removal functionality to be added in this patch at the same time as basic symbol functionality. That should be a different patch, and can then include both symbol and section removal. What do you think?

LGTM. You might want @alexshap to confirm before landing this though.

LGTM.

When you've addressed review comments, it's helpful if you mark the "Done" box in the relevant comment. That way it's obvious what hasn't been addressed. If you do it before you upload, then they will get submitted when the patch gets updated.

LGTM! Thanks.

Yup, LGTM.

LGTM.

I don't think you want to be modifying the number of dynamic symbols at all. That would require you to make other changes to loadable code, which would impact addresses, and effectively require you to completely relink the program.

LGTM.

@jakehehrlich/@rupprecht, I looked at this offline with @chrisjackson, and it's something we would find useful. In particular, we're interested in being able to make the new symbols STV_HIDDEN, so that they don't get involved in symbol pre-emption (although I could imagine some users wanting STV_PROTECTED semantics too). Without this patch, there's no way of avoiding this as far as I know.

@jakehehrlich/@rupprecht, I looked at this offline with @chrisjackson, and I think it makes sense to add this functionality. I believe it's above what GNU does (at the moment), but I can't think of any reason why a user shouldn't be allowed to add STV_PROTECTED symbols, and we have a use-case for it internally.

Aargh, you pushed your latest update whilst I was doing my last review, so I can't see what are the latest changes!

LGTM.

LGTM, with one nit.

In D65847#1618644, @MaskRay wrote:

In D65847#1618571, @jhenderson wrote:

In D65847#1618471, @MaskRay wrote:

In D65847#1618466, @jhenderson wrote:

LGTM. I assume that the 64-bit note layout (i.e. one with a 64-bit size field) works too?

binutils/readelf.c

printf (_("  %-20s %10s\tDescription\n"), _("Owner"), _("Data size"));

I think it is very unlikely that the size is larger than 32-bit...

I agree a large size is unlikely. I was more concerned with it adding leading zeroes (correctly/incorrectly) for 64-bit numbers if the format was 64-bits. That broke the layout of some other tools, if I remember correctly.

typedef struct
{
  Elf64_Word n_namesz;			/* Length of the note's name.  */
  Elf64_Word n_descsz;			/* Length of the note's descriptor.  */
  Elf64_Word n_type;			/* Type of the note.  */
} Elf64_Nhdr;

We don't have to worry about that. n_descsz is 32-bit in both ELFCLASS32 and ELFCLASS64.

You missed one that got added very recently! See the FIXME introduced in rL367942.

In D65847#1618471, @MaskRay wrote:

In D65847#1618466, @jhenderson wrote:

LGTM. I assume that the 64-bit note layout (i.e. one with a 64-bit size field) works too?

binutils/readelf.c

printf (_("  %-20s %10s\tDescription\n"), _("Owner"), _("Data size"));

I think it is very unlikely that the size is larger than 32-bit...

LGTM. I assume that the 64-bit note layout (i.e. one with a 64-bit size field) works too?

CC'ing @wolfgangp, due to D65787 adding a new switch. One of the two patches will need updating once the other lands.

CC'ing mmpozulp due to him writing the llvm-strip doc in D65384. This option will need adding, assuming this patch lands first.