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

Windows build fails: undeclared identifier 'close' #19

Closed
ghost opened this issue Jul 27, 2015 · 6 comments
Closed

Windows build fails: undeclared identifier 'close' #19

ghost opened this issue Jul 27, 2015 · 6 comments

Comments

@ghost
Copy link

ghost commented Jul 27, 2015

Originally reported on Google Code with ID 19

There appears to be a weird mix of both open() and fopen() (with corresponding close()
and fclose()) in cld2_dynamic_data_loader.cc, and possibly other places in the code.
We should consistently use one or the other. To use close() we'd also technically need
to depend on unistd.h, I think, which we currently don't. This is causing some problems
for Chromium, though why it has just cropped up now I could not say:

https://code.google.com/p/chromium/issues/detail?id=403222

The fix here should be trivial, and I'll take care of it now.

Reported by andrewhayden@google.com on 2014-08-13 09:41:53

@ghost
Copy link
Author

ghost commented Jul 27, 2015

So it wasn't quite as trivial as hoped, because mmap wants a file descriptor (int) instead
of a FILE*. Using fopen() and fclose() is the portable way to do things, but under
the covers they do a bunch of buffering and such that we don't really need for our
mmap call. On the bright side it seems like using fileno() from stdio.h allows us to
get the file descriptor, and I can't imagine that we will have any problems with mmap'ing
the region read-only so long as we are also fopen()'ing in "r" (read-only) mode, even
if it mmaps the same region under the covers. So hopefully all is well.

All unit tests pass in both dynamic and non-dynamic modes with the attached patch,
so I think everything should be alright.

Reported by andrewhayden@google.com on 2014-08-13 10:02:42


- _Attachment: [patch](https://storage.googleapis.com/google-code-attachments/cld2/issue-19/comment-1/patch)_

@ghost
Copy link
Author

ghost commented Jul 27, 2015

I had a pro/con discussion with another engineer on this, and here is the summary: the
patch will fix the problem, but is a little sketchy because of the mixing of fopen()
with mmap(). It would be better to #include unistd.h, and continue using open() instead
of fopen(); but this is specific to Unix and wouldn't work on Windows. That said, the
code is *already* broken on Windows because of the use of sys/mman.h (for mmap).

So: If we use unistd.h, we deepen the problems for Windows. If we use fopen(), we do
something questionable for non-Windows (it works, but it isn't really the right thing
to do).

I think the answer here is to use unistd.h for linux and use a workaround like this
to fix win32:

// Based on definitions from http://sourceforge.net/p/predef/wiki/OperatingSystems/:
#ifdef _WIN32
#include <io.h>
#define OPEN _open
#define CLOSE _close
#else
#include "unistd.h"
#define OPEN open
#define CLOSE close
#endif

We can extend this solution to fix the mmap problem as well, in the near future.

Reported by andrewhayden@google.com on 2014-08-13 11:12:51

@ghost ghost self-assigned this Jul 27, 2015
@ghost
Copy link
Author

ghost commented Jul 27, 2015

I've created issue 20 to track the overall Windows compatibility problem for the dynamic
data loader.

Reported by andrewhayden@google.com on 2014-08-13 11:40:31

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Here is a new patch based on #2 above. As described in issue 20, it disables support
for win32 in the "file"-based dynamic data apis; only raw pointer mode will work on
win32 until we have proper mmap support there.

So to be clear, we didn't end up changing the open() to fopen(), because open() makes
sense for this use case. Updating the bug description accordingly.

Reported by andrewhayden@google.com on 2014-08-13 12:31:26


- _Attachment: [patch2](https://storage.googleapis.com/google-code-attachments/cld2/issue-19/comment-4/patch2)_

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Should be fixed in r166.

Reported by andrewhayden@google.com on 2014-08-13 12:35:43

  • Status changed: Fixed

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Issue 23 has been merged into this issue.

Reported by andrewhayden@google.com on 2014-08-29 11:33:08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant