User Details
- User Since
- Jul 25 2016, 12:54 PM (234 w, 4 d)
Today
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.
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.
Wed, Jan 20
Tue, Jan 19
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.
Mon, Jan 18
Sun, Jan 17
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?
Fri, Jan 15
LGTM
LGTM fwiw, I just looked into the same yesterday, but I guess @ldionne should ack it too.
Thu, Jan 14
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?
Wed, Jan 13
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.
Tue, Jan 12
Mon, Jan 11
Sat, Jan 9
Fri, Jan 8
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.)
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.)
Thu, Jan 7
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.
Mon, Jan 4
Wed, Dec 30
Dec 20 2020
The original, unreduced testcase seems to work fine now at least - thanks! Can't really comment on the code itself though.
Dec 18 2020
Rebased, added a comment pointing out suboptimal cases of iterating over the strings twice.
Can you @curdeius have a look at this one?
Ping @amccarth - this requires your follow-up
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?
Dec 17 2020
Ping @amccarth, do you want to follow up on the discussion before I land this?
Ping @amccarth