Thank you for the fix, @alexshap!

Fixed by @alexshap in D78946.

Nitpick in summary: Perhaps you might want to refer to D63847 instead of D63746?

LGTM once clang-format warnings are addressed and build bots are happy.

In D71647#1970793, @alexshap wrote:

Just in case: I've prepared a diff which adds support for relocations (for MachO) to ObjectYAML / yaml2obj / obj2yaml. I'm planning to send it for code review soon (today or tomorrow).

Sorry for being too late. I have no sufficient time to work on this until the end of this month.

BTW, I noticed that we need a same fix for n_sect field in nlist (in a separate patch).

Update a comment in test. NFC.

Merged into D66283: I think it does not have to be a separate patch because using StringSaver simplified what I wanted to do in this patch.

• Merge D71331
• Use StringSaver as suggested in D71331#1780934
• Address review comments

In D63309#1780984, @MaskRay wrote:

From https://developer.apple.com/documentation/kernel/relocation_info?changes=latest_major&language=ob_7:

r_symbolnum

Indicates either an index into the symbol table (when the r_extern field is set to 1) or a section number (when the r_extern field is set to 0). As previously mentioned, sections are ordered from 1 to 255 in the order in which they appear in the LC_SEGMENT load commands. This field is set to R_ABS for relocation entries for absolute symbols, which need no relocation.

The current Mach-O reader and writer do not handle r_extern=0 relocations.

// MachO/MachOReader
void MachOReader::setSymbolInRelocationInfo(Object &O) const {
  for (auto &LC : O.LoadCommands)
    for (auto &Sec : LC.Sections)
      for (auto &Reloc : Sec.Relocations)
        if (!Reloc.Scattered) {
          auto *Info = reinterpret_cast<MachO::relocation_info *>(&Reloc.Info);
          Reloc.Symbol = O.SymTable.getSymbolByIndex(Info->r_symbolnum);  //// this can reference a section when r_extern=0
        }
}

In D66283#1779295, @jhenderson wrote:

There seems to be some stuff to do with 64-bit versus normal segments in here, which I don't fully follow due to it being Mach-O, but your test only seems to handle the 64-bit segment type. Should it be extended?

Added 32-bit object support.

Addressed review comments.

Added a newline at EOF.

Addressed review comments and isolate some changes into D71331.

Addressed a comment.

I'll commit this patch on next Monday.

Rebased. NFCI.

LGTM too. Thanks for the fix!

LGTM too.

Updated a test.

Ping @alexshap (and also @jhenderson and @rupprecht would want to confirm before landing). After this patch got accepted, in order to merge it, I added wildcard support and a test in this diff.

Before committing this patch, I noticed that we need to consider how llvm-objcopy should handle wildcards in Mach-O support.

Thank you for your review!

@alexshap Could you review this patch? It's accepted but I'd like someone who's familiar with Mach-O to confirm (a ping in case you missed changes in this patch).

Friendly ping: I'd like someone who is familiar with Mach-O to review this patch.

Rebased.

Rebased. No changes intended.

• Added a comment.

Addressed a review comment.

clang-formatted

Addressed review comments.

Addressed a review comment.

Add MachOConfig.cpp to isolate command-line validation logic from MachOObjCopy.cpp.

Commented regarding tests and nits. Could you update docs/CommandGuide/llvm-objcopy.rst as well?

• Addressed a review comment.

• Added a comment for parseELFConfig.

• Adopt @alexshap's idea (D67139#inline-607235).

Thanks you for feedbacks. Putting it all together, the @alexshap's suggestion (D67139#inline-607235) looks desirable to me because:

In D67139#1670164, @jakehehrlich wrote:

What about having ELFcopyConfig inherit from CopyConfig?

How about this design? I believe moving the CopyConfig is not a big cost.

Pass only ELFCopyConfig instead of passing both CopyConfig and ELFCopyConfig.

Adopt @jhenderson's idea D67139#1657802.

• Removed an unrelated change from the diff.

In D67060#1664226, @ruiu wrote:

This patch looks good, but I'd add a test for the new code. Is there any place in LLVM where the current color is saved and restored? If there's such code, you can rewrite the code to see if your code would work as expected.

Addressed review comments

Addressed review comments.

• Abandon WithColorContext: save the previous color settings in WithColor instead.
• Update the description.

In D67139#1657802, @jhenderson wrote:

I've got another suggestion: have separate COFFConfig/ELFConfig etc classes that only contain the interpreted versions and lazily construct them prior to calling the corresponding code path. CopyConfig would contain the raw unparsed strings (perhaps rename them to "RawSymbolsToAdd" etc). Perhaps a higher-level Config class could exist just to wrap them and pass them to executeObjcopyOnBinary etc.

Rough outline:

CopyConfig.h:

class CopyConfig {
  std::vector<StringRef> RawSymbolsToAdd;
  ...
};

class ELFConfig {
  std::vector<NewSymbolInfo> SymbolsToAdd;
  ...
};

// etc for COFF, Mach-O...

llvm-objcopy.cpp:

struct Configurations {
  CopyConfig Common;
  Optional<ELFConfig> ELF;
  ...
}

...

static Error executeObjcopyOnBinary(Configurations &Cfgs, ...) {
  if (/*isElf*/...) {
    if (!Cfgs.ELF)
     Cfgs.ELF.emplace(Cfgs.Common);
    return elf::executeObjcopyOnBinary(Cfgs.Common, Cfgs.ELF, ...)
  }
  ...
}

LGTM. I'll implement the idea.

In D67060#1656873, @ruiu wrote:

I don't think there's a way to get the current color from the terminal, so you should modify changeColor so that the function remembers the color given to the function.

In D67060#1656821, @ruiu wrote:

This looks better, but I don't think we need the concept of context. If a function takes a context object, it is usually expected that you can interlace function calls with different context objects freely. Say, you have context object A and B, then you literally have two contexts, and they don't interfere with each other. However, that's not the case for the terminal color. The current color of text output on console is naturally and fundamentally a global state associated to each terminal, and therefore there is only one context. So, I don't think that allowing multiple contexts captures the real concept correctly.

I believe the following scheme models captures the reality better:

1. Add a new method, getColor, to raw_fd_stream, which returns the current color set by changeColor before
2. Make a change to WithColor so that it remembers the current color (obtained by calling getColor) before changing color
3. In the destructor, WithColor restores the previous color

In D66281#1656482, @alexshap wrote:

@seyia, sorry about the delays with code review, does this particular diff have any unreviewed dependencies ?

In D67060#1655157, @ruiu wrote:

Did you find any existing usage of WithColor that depends on the behavior of resetting the color? If not, you don't have to care too much about keeping the compatibility, as that is not really an intended feature as far as I understand. You probably can just change the behavior of WithColor so that it restores to the previous color instead of resseting.

Moved NestableWithColor features into WithColor.

In D67060#1654353, @jhenderson wrote:

In D67060#1654259, @ruiu wrote:

Instead of adding a new class, do you think you can change the behavior of WithColor so that it restores the original colors instead of resetting it?

This would be my preference too, if it's viable. I don't think WithColor has been around that long, so I suspect it may be straightforward to identify existing usage patterns, and I'd be surprised if any expect restoration to default.

Added reviewers who may be familiar with this part.

This patch does not include tests but it will be used in D65191.

In D66449#1648463, @seiya wrote:

In D66449#1646539, @jhenderson wrote:

Test cases for the new code?

I forgot to add NFC in the title. This patch does not introduce new features thus we don't need new tests for it I think.

In D66449#1646539, @jhenderson wrote:

Test cases for the new code?

Addressed a review comment.

Addressed review comments.