LGTM

In D122258#3479045, @int3 wrote:

Thanks for the feedback!

The binary compatibility issue is non-existent on arm64, but it could be still a thing on x86_64.

Can we enable OmitDwarfIfHaveCompactUnwind by default on arm64 then? And have it be opt-in just for x86_64?

Compact unwind was developed as a goal to replace the DWARF unwind. Some apps did not work if the OS was missing DWARF unwind, so we kept both for Intel. The binary compatibility issue is non-existent on arm64, but it could be still a thing on x86_64.
If a function cannot be encoded in CU, we need the compiler to emit both (where the CU is a magic value that points to the dwarf unwind info for that function).

In D121159#3366232, @jhenderson wrote:

In D121159#3366227, @davide wrote:

In D121159#3366196, @jhenderson wrote:

The pre-merge check is failing to build this patch.

There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:

I think this is for UNIX03 compliance. Not all the tools have to be compliant.

Perhaps not, but there's presumably a good reason for compliance requirements. Anyway, I'd be surprised if llvm-nm is any more special than e.g. llvm-readelf or llvm-objdump in this area.

2. stdout is flushed on exit automatically: https://stackoverflow.com/questions/15911517/is-there-a-guarantee-of-stdout-auto-flush-before-exit-how-does-it-work

If stdout is flushed before exit, that's fine. What about stderr ?

stderr is unbuffered. It shouldn't need flushing.

How does explicit flushing help us with output errors. More importantly, why do we care?

We care for POSIX compliance. Apple can keep this patch downstream, but I don't see the harm of having this one here.

Harm comes from having code that is unnecessary, since it does nothing that doesn't automatically happen. Future developers will come along and see it and delete it because it appears to be pointless. You didn't answer the first half of that comment though: how does explicit flushing help us? The patch description talks about error checking a stream, but this modification doesn't seem to do anything of the sort.

In D121159#3366196, @jhenderson wrote:

The pre-merge check is failing to build this patch.

There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:

I don't run POSIX compliance, somebody just gave me a patch, but yes, good point.

We fixed this in the linker.

In D112124#3080812, @steven_wu wrote:

The best solution might still be the linker to map the intermediate file to the original object file since the added file name here is still another level of indirection you need to figure out. @davide for his opinion.

removing davide
+ steven

Sure.

+ Lang, who probably has a better understanding of this, for visibility.
I don't think this should be blocked further, but he might have a chance to comment.

LGTM.

In D98458#2620731, @MaskRay wrote:

Mach-O doesn't support dso_local and this change broke XNU because of the use of dso_local.

Can you elaborate on the unsupportness? For example, the backend can ignore dso_local if it does not want to support it.

The main purpose is to reduce target differences: currently both COFF and ELF can use dso_local. If Mach-O doesn't like dso_local, there is an additional difference.

I apologize, but I don't work on this anymore.

I'm sorry, I haven't touched this code in a long time and I don't feel qualified to review it.

Committed thusly

This landed in a different form.

Somebody will pick this up, probably in a different form.

I won't have time to look at this in the future.

@labath something I noticed when finding this (and related bugs) is that frame var carries a decent diagnostic

I came to the same conclusion when analyzing https://bugs.llvm.org/show_bug.cgi?id=47257 (but you beat me to the punch).

I think this is fine. @mehdi_amini ?

In D85396#2198668, @tschuett wrote:

auto_ptr is deprecated since C++11 and will be removed in C++17.

This is correct to the best of my understanding. Thank you Jason.

yes, this makes sense. We could refine the check in future.

I assume we're still allowing to put the decorator on a test-by-test basis, and that seems the case from what I see.
If so, LG.

LGTM with Saleem's comment addressed.

https://github.com/llvm/llvm-project/commit/57605758b5de3726eec1d6e587de1003af1ab5b7

Reviewed by Jason privately.

Added the check that Jason requested.

Florian, can you take a look?

why? Do you have a testcase?

yeah, I think it's reasonable.