nice!

lgtm

We bisected some Chromium test failures on Android, which points to this change (https://crbug.com/1270361). We'll investigate further, just wanted to give a heads up.

lgtm

lgtm

In D110368#3131219, @hans wrote:

A Chromium developer reported compiler non-determinism that we bisected to this change. see https://bugs.chromium.org/p/chromium/issues/detail?id=1268247#c12 for a reproducer with preprocessed source.

lgtm

lgtm

At what point does EmitGCCInlineAsmStr and EmitMSInlineAsmStr become similar enough that they could just take the desired variant as a parameter?

Sorry, the attached file is here:

I am not sure how to reproduce this.

We're seeing a diagnostic change in Chromium which looks funny. For the following code (span<> is our own class):

Nice!

Thanks! lgtm

lgtm

I'm not familiar with this code, so apologies for the silly question, but how does this change make a difference for the new test case?

creduce finished, posted the reduced input here: https://bugs.chromium.org/p/chromium/issues/detail?id=1270222#c5

Looks reasonable to me, but every time I touch anything related to inline asm, something always breaks, so I'm also a bit worried.

A Chromium developer reported compiler non-determinism that we bisected to this change. see https://bugs.chromium.org/p/chromium/issues/detail?id=1268247#c12 for a reproducer with preprocessed source.

This caused assertions in Chromium builds targeting Windows. See https://bugs.chromium.org/p/chromium/issues/detail?id=1270222#c1 for a reproducer.

lgtm

The reproducer I was using looks good with the latest version of the patch. Thanks!

I've reverted back to green in https://github.com/llvm/llvm-project/commit/a2a58d91e82db38fbdf88cc317dcb3753d79d492 until this can be fixed.

This seems to have caused a miscompile of Chromium, see https://bugs.chromium.org/p/chromium/issues/detail?id=1265339 (there's even a screenshot).

lgtm with one question

lgtm

lgtm

Feel free to ignore the comment.

lgtm

Sgtm.

Nice!

Do you think the ObjCAtTryStmt::Create and CreateEmpty methods could be simplified by using totalSizeToAlloc()?

lgtm, just some nits.

lgtm

In D112081#3073067, @aaron.ballman wrote:

Thanks for this! Should we similarly handle __STDC_NO_ATOMICS__, __STDC_NO_COMPLEX__, et al at the same time?

Sounds good to me :)

I guess I don't have enough context here. Why is AttributedStmtClass special here? Is it because it's a statement and the others are expressions, and so wouldn't go through CFGBlock::addStmt()?

Slightly better diags, try to appease clang-format.

lgtm

Nice. Might be worth mentioning in Clang's release notes too.

If you want to look into it, I think having some kind of test for this script would be useful.

Is there any codegen test we could update to check that a high alignment value makes it all the way through to the IR?

Are there no tests for clang-format-diff.py? That seems unfortunate.

Sounds very reasonable to me. lgtm

In D111072#3042430, @probinson wrote:

Is there a config tweak needed to get this to run properly for us? Given the upstream Windows bots are not seeing a problem.

We're also hitting the error in a downstream build (https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1808), and it reproduces for me locally.

This looks good to me. The only downside I could see is if we'd be growing sizeof(Instruction), but it sounds like that's not happening.

lgtm

lgtm, thanks for digging into this!

Nice! lgtm

Nice!

lgtm

lgtm

In D110290#3021030, @nikic wrote:

@hans Do I understand correctly that this change was mistakenly reverted and can be reapplied now that the DSE issue is fixed?

This caused compiler crash when building Chromium. See https://bugs.chromium.org/p/chromium/issues/detail?id=1252762#c8 for a reproducer, and the top of that bug for the stack dump.

In D109828#3002495, @thakis wrote:

In D109828#3002114, @hans wrote:

The /winsysroot part makes sense to me, but what's the case for the new /diasdkdir flag?

• If you do have a vcvars shell, you don't need the full sysroot path and it's kind of useful (see example in commit message)
• it seems nice to be able to explain /winsysroot as combination of other flags in the help text
• it makes writing the test a bit easier
• it makes diasdkdir more like the other flags controlled by /winsysroot

The /winsysroot part makes sense to me, but what's the case for the new /diasdkdir flag?

Commit message nit:

lgtm

In D109624#2995336, @thakis wrote:

I wonder if there are deeper reasons for GCC and Clang's current behaviour than early diagnosis and slightly nicer error messages.

One easy way to find out is land this and if it breaks something, add a comment and a test for the next person :)

Since cl.exe seems to match the behaviour your patch is switching to, maybe we should only do this in clang-cl mode?

Maybe! But the current behavior is arguably wrong for the clang -fuse-ld=lld -Wl,-chroot,some/path /foo.o case, so maybe we should try changing both for starters and then row back if needed?

Hmm. As you worried yourself in https://bugs.llvm.org/show_bug.cgi?id=27234#c3 we might be getting a worse user experience in some cases. For example:

In D109361#2986687, @hans wrote:

Thanks! I tried this locally and it fixes the issue I mentioned on D109361

lgtm, thanks!

lgtm

Thanks! I tried this locally and it fixes the issue I mentioned on D109361