Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of try-except within MinGW/Clang on Windows #2407

Closed
Crunkle opened this issue Aug 6, 2019 · 6 comments

Comments

@Crunkle
Copy link
Contributor

commented Aug 6, 2019

Neither GCC nor Clang fully support the try-except MSVC extension and so any builds as of 2c27950 fail in non-MSVC Windows environments. I have seen some dirty hacks to implement SEH outside of MSVC but they are far from portable.

See the use of __try and __except within win/fs.c, also see here for reference.

I am not sure what the best solution is, but I don't see reason for the memcpy to fail. For that reason I want to propose something simple:

diff --git a/src/win/fs.c b/src/win/fs.c
index 83dfdca9..b2835628 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -786,6 +786,7 @@ void fs__read_filemap(uv_fs_t* req, struct uv__fd_info_s* fd_info) {
     int err = 0;
     size_t this_read_size = MIN(req->fs.info.bufs[index].len,
                                 read_size - done_read);
+#ifdef _MSC_VER
     __try {
       memcpy(req->fs.info.bufs[index].base,
              (char*)view + view_offset + done_read,
@@ -797,6 +798,11 @@ void fs__read_filemap(uv_fs_t* req, struct uv__fd_info_s* fd_info) {
       UnmapViewOfFile(view);
       return;
     }
+#else
+    memcpy(req->fs.info.bufs[index].base,
+           (char*)view + view_offset + done_read,
+           this_read_size);
+#endif
     done_read += this_read_size;
   }
   assert(done_read == read_size);
@@ -978,6 +984,7 @@ void fs__write_filemap(uv_fs_t* req, HANDLE file,
   done_write = 0;
   for (index = 0; index < req->fs.info.nbufs; ++index) {
     int err = 0;
+#ifdef _MSC_VER
     __try {
       memcpy((char*)view + view_offset + done_write,
              req->fs.info.bufs[index].base,
@@ -989,6 +996,11 @@ void fs__write_filemap(uv_fs_t* req, HANDLE file,
       UnmapViewOfFile(view);
       return;
     }
+#else
+    memcpy((char*)view + view_offset + done_write,
+           req->fs.info.bufs[index].base,
+           req->fs.info.bufs[index].len);
+#endif
     done_write += req->fs.info.bufs[index].len;
   }
   assert(done_write == write_size);

Reproducible on almost all non-MSVC Windows builds. Possibly fixes or related to #2391?

@saghul

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Yeah, I don't see why we need those either. We are not using this anywhere else IIRC.

Ping @libuv/windows

@cjihrig We may want to fix this before release.

@bzoz

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

One of the buffers is a memory-mapped file. If something fails while accessing the hard drive, it will be reported as an exception.

I'm OK with removing the __try block for non-MSVS compilers, with a note in the documentation that using memory mapped files can crash the program when libuv is compiled with those.

@saghul

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

SGTM. Didn't we have a thing to disable CRT exceptions? Or is that something else?

cjihrig added a commit that referenced this issue Aug 7, 2019

test: fix test runner on MinGW
MinGW does not export the _set_fmode function via its
libmsvcrt.a import library unless on an ARM platform. This
causes the test target build to fail without manually
adjusting the link parameters.

It is safe to assume that _fmode is available in all stable
releases, and it should be preferred unless using MSVC. This
is unrelated to #2407, but when both are fixed, MinGW builds
should complete successfully.

PR-URL: #2411
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I'm currently blocking the 1.31.0 release on this and one other patch. Is anyone currently working on (or planning to work on) this?

@saghul

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

I think @Crunkle 's proposal is good. If they are not able to PR it I can.

@Crunkle

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

PR created, thanks!

I'm not too familiar with the possible exceptions but I agree, either a warning pragma or, even better, a small note in the docs could be a good addition.

@cjihrig cjihrig closed this in 813264a Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.