Steven, Let's discuss this offline and come back with a proposal.

LGTM

LGTM

In D140224#4014256, @MaskRay wrote:

In D140224#4014243, @rsundahl wrote:

I'll reject [-\Xparser for a while as well. This is a valid amendment to D139717 , so I don't think it needs more approval.

We have projects that are failing because of -Xlinker and I'm not too excited about walking through every project we have to find more examples. Can we please have a reprieve on deprecating this until every project in the world that uses these flags that have existed for a very long time can have a chance to at least get timely notification? Why is this any different than anything else that gets deprecated?

Thank you for your reply! If there are more issues -Xlinker/-Xparser/-Xcctests, ok, I think I'm fine with a Joined -X, but ideally we can figure out a way to apply that only to Apple platforms.

(FWIW I feel sad when I made valid objection to D139717, but that patch landed very soon after your colleague approved it, leaving me no time to object.
My other objection included the insufficient commit message -Xfoo instead of what's actually used, internal builds which seems to hint an internal workaround, where the tests are located)

In D140224#4014245, @MaskRay wrote:

In D140224#4014234, @davide wrote:

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.

Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that such -X* has -Wunused-command-line-argument warning for many many years.
I'm not sure how is considered insufficient.

"Working around 3 cases creates more complexity than it fixes." the number isn't that high. By enumerating the misuse, we have a valid path to remove all workarounds as misuses are fixed.
This made some forward progress.

In D140224#4014230, @MaskRay wrote:

In D140224#4014203, @davide wrote:

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

D139717 (this patch reverts) was pushed when I made valid comments which were ignored. I did not complain for that.

I don't mind if you work around -Xcctests in a similar way.

@MaskRay Roy hasn't replied. We found other spellings that break (e.g. -Xcctests or something). Revert this patch until we find an agreement.

Fine if Roy is happy. If we find another thing that break, we'll let you know. Please wait for him to comment.

In D139717#4001702, @MaskRay wrote:

In D139717#4001688, @davide wrote:

In D139717#4001685, @MaskRay wrote:

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

This is not about the philosophical correctness of the patch, it's about the transition period and allowing consumers to migrate.
If you want remove options, provide a deprecation window, and then remove. Noone is objecting about that.

-Xparser has always been leading to such a warning: warning: argument unused during compilation: '-Xparser' [-Wunused-command-line-argument], perhaps since forever when the option was added in the first place?
The message is different from ... deprecation ... but isn't it sufficient as well?

In D139717#4001685, @MaskRay wrote:

In D139717#3998077, @manojgupta wrote:

Xlinker still works. Xcompiler is failing.

A google search will show that Xcompiler is a wide-spread option used by many packages. Whether or not GCC supports it is not relevant. Please do not remove options just because you do not use them.

Can you give an example how they use -Xcompiler?

% gcc -Xcompiler,-fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
% gcc -Xcompiler -fpic -c a.c
gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean ‘--compile’?

My commit message clearly says why the joined form is awkward and should be removed.
It seems that the many occurrences you found are likely for GNU libtool (-Xcompiler foo -Wc,foo), not for Clang Driver.

LGTM

In D139717#3997193, @MaskRay wrote:

In D139717#3991765, @rsundahl wrote:

In D139717#3987963, @MaskRay wrote:

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.

We use -Xparser

There are two possibilities.

First, your downstream projects incorrectly pass a previously-ignored -Xparser. Then it seems the right call is to remove it.

In D139717#3987963, @MaskRay wrote:

This change is breaking internal builds. We use the -Xfoo pattern but can now no longer manage whether we allow an unused -Xfoo option to pass as a warning or promote it to an error.

Which -Xfoo is used? Do your internal builds reserve -Xfoo as a placeholder to do other non-upstream work? Then you can consider -Gfoo if your builds don't need the mips semantics.
Is it possible to clean up the previously-ignored-with-a-warning -Xfoo?

In D139717#3984648, @lebedev.ri wrote:

This is missing a test, like the original commit mentioned.
Why can you not just use the the split variant, -X clang ...?

@t.p.northover regardless of whether this meets the bar, this is a test case that takes forever reduced from a real project.
We should have a solution upstream and this is the purpose of the patch. Downstream can't live with hacks forever.
Do you have any thoughts?

An interesting alternative might be limiting this to when we're optimizing for size. Not sure if it sounds good for you, but maybe it's better than a cl::opt.

In D138602#3979174, @aeubanks wrote:

sorry, this fell between the cracks

are the alwaysinline attributes added purely for optimization purposes (counterexample, I've seen cases for testing where they are to make sure the number of frames is consistent)? if so I'd try removing literally every alwaysinline attribute and seeing what performance difference that makes

@aeubanks @davidxl can we get some movement on this one, please?

LGTM, again.

LGTM with the comment reworded. Thanks.

Exprs.size() == 1 is still valid in every example we've seen, so yes, you might want to keep it (and update the assertion message)

@aeubanks Gentle ping

@RKSimon / @pengfei ping

To elaborate further, the testcase in the project issue reported is reduced from a preprocessed file that's approximately 1 million lines.
Even if people would like the remove always_inline -- it's very hard to understand where to start (and, realistically, it's a jenga tower of optimizations + templates).

Hi @aeubanks -- admittedly I haven't touched the inliner in a very long time so I might be wrong here, but my understanding is that alwaysinline trumps the cost multiplier (that's why the previous patch is not enough).

Hey folks, this is still not enough. We're seeing exponential behaviors when linking real-world projects.

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.