In D93764#2515922, @mstorsjo wrote:

Thanks! I think it looks mostly good now - I'm still seeing some breakage compared to thursday, but I'll have to bisect those errors separately to see where they broke.

In D93764#2515449, @fhahn wrote:

In D93764#2515106, @mstorsjo wrote:

I've got one problem narrowed down to one source file in a project; https://martin.st/temp/dav1d-thread_task-preproc.c, compiled with clang -target x86_64-w64-mingw32 -c -O3 does trigger the issue - I haven't dug in closer to zoom in on exactly what it is though - hopefully you can spot the differences.

One issue with the file was that the patch did not account for atomic loads and would add additional atomic loads. I fixed that in 86991d323133. Please let me know if that fixes your issue. Otherwise I'll revert the patch to investigate further.

I've got one problem narrowed down to one source file in a project; https://martin.st/temp/dav1d-thread_task-preproc.c, compiled with clang -target x86_64-w64-mingw32 -c -O3 does trigger the issue - I haven't dug in closer to zoom in on exactly what it is though - hopefully you can spot the differences.

In D67954#2515028, @mgorny wrote:

Is there a specific reason you've only covered x0..x7 in the test?

I'm also seeing lots of hangs/infinite loops in code built after this commit. I don't have anything nicely reduced to dig in on (yet), but it broke a significant amount of my testing setup.

In D77609#2511477, @tianshilei1992 wrote:

@mstorsjo Would you mind giving it a shot on Windows?

Made a readlink helper in posix_compat.h.

Made fchmod and fchmodat helpers in posix_compat.h.

Made a new realpath posix-like helper for windows, change explicit char to path::value_type in calling code.

Moved to a getcwd helper in posix_compat.h, change existing code to use path::value_type instead of hardcoded char.

Retrigger CI.

Rebased, moved helpers to posix_compat.h.

Retrigger CI.

Rebased on top of updated patch (already preapproved, no need for new review).

Retrigger CI.

Moved posix reimplementations to a separate header.

FWIW, +1 from me on this one. I've earlier run into cases where code checks for whether <filesystem> is available and works, where it has accidentally seemed to work as long as the test only used features provided entirely by the header. See e.g. https://codereview.qt-project.org/c/qt/qtbase/+/295469 for such a case.

This broke building OpenMP for windows; all the new helper functions, like __kmp_hidden_helper_threads_initz_wait, that are added in z_Linux_util.cpp would need to be added similarly to z_Windows_NT_util.cpp. What do you propose doing - revert the patch for now until that's in place?

LGTM

LGTM fwiw, I just looked into the same yesterday, but I guess @ldionne should ack it too.

In D94707#2500752, @ChinouneMehdi wrote:

In D94707#2500607, @mstorsjo wrote:

In D94707#2500579, @ChinouneMehdi wrote:

In D94707#2500131, @mstorsjo wrote:

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

D:/dev/llvm-project/flang/runtime/file.cpp:48:57: error: use of undeclared identifier '_S_IREAD'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                        ^
D:/dev/llvm-project/flang/runtime/file.cpp:48:68: error: use of undeclared identifier '_S_IWRITE'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};

The problem here is just that the right header for the _S_IREAD and _S_IWRITE constants haven't been included. Include sys/stat.h (which exists in both MSVC and mingw environments), i.e. move that include out of the ifdef/else, and this should work for mingw too.

This is the easiest fix I could find.
You are free to make modifications on my patch.

In D94707#2500579, @ChinouneMehdi wrote:

In D94707#2500131, @mstorsjo wrote:

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

D:/dev/llvm-project/flang/runtime/file.cpp:48:57: error: use of undeclared identifier '_S_IREAD'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                        ^
D:/dev/llvm-project/flang/runtime/file.cpp:48:68: error: use of undeclared identifier '_S_IWRITE'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

This broke compilation of some sources, reproducible like this:

$ cat repro.c 
a;
b(long long c) {
  if (c & 5)
    return c;
}
d(c, e) {
  int f;
  int *g = a, *h = d;
  f = 0;
  for (; f < c; f++)
    g[f] = b((long long)h[f] * e);
}
$ clang -target aarch64-linux-gnu -c repro.c -O2
fatal error: error in backend: Cannot select: t85: v2i32 = AArch64ISD::DUP t15
  t15: i64,ch = CopyFromReg t0, Register:i64 %1
    t14: i64 = Register %1

Added clarifying comments in the header, describing why we're providing our own stat implementation, and that the C runtime headers provide possibly conflicting defines that we undef.

In D93190#2485484, @MaskRay wrote:

@mstorsjo Perhaps you can test this as well:)

In D94364#2490743, @compnerd wrote:

Can you please add some more context around the motivation for this change? It seems ... unnecessary as everything is already AAPCS/AAPCS-VFP.

In D91176#2488251, @amccarth wrote:

I'd like to see a name change to clear up the consumeSeparators confusion, but I won't block on that.

Readd the repo, for CI...

Renamed the consumeSeparators method to consumeNSeparators and added a comment pointing out that it consumes exactly that amount of separators. Removed unnecessary braces around single lines. (The consumeSeparator method can be renamed in a separate patch afterwards, I'd avoid doing it here to keep the diff as readable as possible.)

In D91176#2486783, @curdeius wrote:

I may be repeating myself, but my biggest remark is that there are no tests. I know there's no libcxx CI on Windows, but still, that will make reviewing easier, looking what are the new test cases and what has been forgotten.

This one is still unreviewed, and is more of generic libcxx code than windows specific API calls - could @curdeius have a look?

This caused misoptimizations for armv7, where code that previously worked correctly now produce different results. (The code is clean under ubsan, so it shouldn't be relying on anything undefined.)

In D93186#2485140, @mstorsjo wrote:

FWIW, while I've touched this code on the outside, I'm not really familiar with it. But I can at least try out the patch in my setups (dwarf + SEH) and let you know whether it seems to work as intended there.

Ping - as @efriedma doesn't seem to be around much at the moment, could maybe @rnk give this one an ack?

FWIW, while I've touched this code on the outside, I'm not really familiar with it. But I can at least try out the patch in my setups (dwarf + SEH) and let you know whether it seems to work as intended there.

The original, unreduced testcase seems to work fine now at least - thanks! Can't really comment on the code itself though.

In D87188#2463733, @mstorsjo wrote:

In D87188#2463566, @lebedev.ri wrote:

Once again, thank you @mstorsjo and @dmgreen!

Awesome, thanks guys!

I'll let you know tomorrow in the unlikely case if this still caused some other regressions as well (or possibly later, if my nightly build ends up broken for other reasons).

In D87188#2463566, @lebedev.ri wrote:

Once again, thank you @mstorsjo and @dmgreen!

Rebased, added a comment pointing out suboptimal cases of iterating over the strings twice.

In D91181#2462705, @curdeius wrote:

LGTM. I'd like just to see a TODO note or a ticket for this double traversal.

In D87188#2461510, @mstorsjo wrote:

In D87188#2461497, @lebedev.ri wrote:

@mstorsjo i've bothered @dmajor to give this a try on some of mozilla's CI, and unfortunately it came back green..
I don't recall if i can reproduce it locally (on vanilla test suite, i'll recheck),
but right now it would seem that you are the only one who can reproduce this :/

Were you able to make any progress on a somewhat more actionable reproducer?

What would be really helpful is a standalone source that can be compiled and run (here by be locally, on linux x86_64)
that produces different result with/without this change.

IIRC I'm only able to reproduce this for aarch64 (and possibly arm) targets unfortunately. I can make a kind of standalone testcase with one single file built from source, linked and executed along with a bigger piece of code (either from source or prebuilt object files), but it'd be for aarch64-linux (or aarch64-darwin if that helps - I haven't tried reproducing it there but I'd presume it behaves the same as I noticed the bug first on aarch64 windows).

In D91138#2462595, @curdeius wrote:

No modifications in tests?

Can you @curdeius have a look at this one?

Ping @amccarth - this requires your follow-up

Ping, this one is still entirely unreviewed - @curdeius @amccarth?

Ping @amccarth, this one is missing follow-up on the earlier discussion.

Rebased, already formally approved.

Rebased, retriggering CI. This one is already formally approved by @amccarth.

Rebased on top of stylistic changes in earlier patches, retriggering CI

Can either @amccarth or @curdeius mark this as formally approved, if there's no other strict blockers for it, to maybe help with getting it looked at?

In D91141#2403239, @amccarth wrote:

LGTM as long as the run-time calculation of the default value for set_errno is legit.

In D87188#2461497, @lebedev.ri wrote:

@mstorsjo i've bothered @dmajor to give this a try on some of mozilla's CI, and unfortunately it came back green..
I don't recall if i can reproduce it locally (on vanilla test suite, i'll recheck),
but right now it would seem that you are the only one who can reproduce this :/

Were you able to make any progress on a somewhat more actionable reproducer?

What would be really helpful is a standalone source that can be compiled and run (here by be locally, on linux x86_64)
that produces different result with/without this change.

Ping @amccarth, do you want to follow up on the discussion before I land this?

Ping @amccarth