In D123831#3458774, @dang wrote:

In D123831#3455048, @cishida wrote:

we might not always want to transform an absolute path because the resulting relative include name might get remapped in a headermap, for example in test known_files_only_hmap.c. But how does it work with modules where we need relative includes? Is the setup in known_files_only_hmap even valid?

I think, in most cases, this shouldn't matter because if the header path input doesn't match the location stored in the header map, they should still have the same source content. The same should be true with header search resolution with modules & vfsoverlay

Agreed, I think it would be classified as a user error to remap to different source content via headermap.

LGTM

Nice! Thank you for adding support for multiple headers. Is this a step towards the json input file list?

LGTM. Just a few minor nits.

LGTM

The unit test failure on Windows seems unrelated, because I have seen it in a previous pre-merge check too. Furthermore, this should be a NFC.

LGTM

LGTM

LGTM

LGTM

LGTM

SpareSet is deterministic given the same input order - similar to SetVector. Printing the result is not the common use case, so changing the print function to sort the output first might be more useful for testing purposes.

Thanks Cyndy. LGTM

Does nm print now all symbols - including the inlined ones? That would be an unexpected change, because we don't do the same for MachOs that have re-exported frameworks. I think this feature should be guarded by an option.

xxhash64 is apparently faster than MD5 and SHA1, and produces good quality hashes. I am not sure about the hash quality of hash_code for this purpose.

LGTM

LGTM

Did you try xxHash64?

LGTM

The motivation to add this to llvm-nm, is to get the same functionally back that we lost with adding TBD files. Using it for testing is a nice side benefit.

LGTM

LGTM, but we should also wait for Lang's feedback.

New version is tracked here: D66149

Sure, np

Hi Cyndy, thanks for catching this copy-past-o bug. LGTM

If I recall correctly, then getIntImmCost(const APInt &Imm) predates the ConstantHoisting pass and is also used by other optimizations. The purpose of that hook is to calculate the materialization cost of a constant and is context free.

Thanks for catching this. LGTM.

LGTM

In D53945#1310580, @jakehehrlich wrote:

Would there be an objection to using llvm::tapi::MachO or llvm::tapi::macho? At least for ELF it doesn't make much sense to put the tapi code in the same namespace as BinaryFormat/ELF so it would be nice to add the extra namespace. I'm not married to the idea however.

• rename setArchitectures to addArchitectures
• fix comments
• use BitmaskEnum in several places to remove custom static_cast code.

• Rebase patch on D53051
• Remove registry and add separate reader and writer classes
• Move everything into the MachO namespace and sub-folder

I will update the patch with more changes based on the feedback so far.

Add missing license header.

We do have that now. All TAPI files start now with "--- !tapi". It is only
that the first version doesn't have that marker and that is why we have to
fall back to this extra check.

Replace std::sort with llvm::sort and fix copy-paste-o bugs.

TAPI mostly cares about linkable symbols, so this shouldn't be a problem.