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

[BUG]show “fcntl(): Bad file descriptor” #1492

Closed
cyz7758520 opened this issue Apr 18, 2021 · 17 comments
Closed

[BUG]show “fcntl(): Bad file descriptor” #1492

cyz7758520 opened this issue Apr 18, 2021 · 17 comments
Assignees
Labels

Comments

@cyz7758520
Copy link

Host OS: Windows
NDK:22.1.7171670

I execute ndk-build.cmd under CMD, it always shows “fcntl(): Bad file descriptor”.

image

@cyz7758520 cyz7758520 added the bug label Apr 18, 2021
@enh-google
Copy link
Collaborator

the internet seems to think that this is caused by the -O option that ndk-build passes to make. tidev/titanium-sdk@60fba0b passes --output-sync=none to avoid it.

it's unclear to me whether this is just an inherent problem on Windows? if so, i guess ndk-build should not pass -O on Windows. but wouldn't we have seen this ourselves?

@DanAlbert
Copy link
Member

DanAlbert commented Apr 19, 2021

My understanding is that this isn't a real error. Everything works correctly, it just also prints this. We see this when we run tests but all the tests behave correctly.

Is this causing a problem that you're seeing, or is the bug here just "it's printing extra stuff"?

@cyz7758520
Copy link
Author

When I use the old NDK 21.4.7075529, there is no such display.

image

@DanAlbert
Copy link
Member

We know, that's when we updated make and started seeing this. It just doesn't appear to cause any harm.

Can you answer the question I asked in the previous comment? Is this causing a problem? We need to know how to prioritize this.

@cyz7758520
Copy link
Author

I haven't found any problems so far, I just want to raise them first to prevent potential problems.

@rprichard
Copy link
Collaborator

I'd be happy to work on this today -- I think I recall mostly debugging it already.

@DanAlbert
Copy link
Member

All yours :)

@cyz7758520
Copy link
Author

Do I need to close the issue now?

@rprichard
Copy link
Collaborator

No, it can stay open.

@rprichard
Copy link
Collaborator

I filed an upstream bug with GNU make: https://savannah.gnu.org/bugs/?60444. I also attached a patch to that bug that I think would fix this issue.

@rprichard
Copy link
Collaborator

FWIW, I tried to build upstream make (with a recent, upstream gnulib) with x86_64-w64-mingw32-gcc, and was unsuccessful. I tried overriding fcntl in configure like so:

export CFLAGS="-Wno-error -Wno-format"
../configure --host=x86_64-w64-mingw32 \
  gl_cv_header_working_fcntl_h=yes \
  ac_cv_func_fcntl=yes \
  gl_cv_func_fcntl_f_dupfd_works=yes \
  gl_cv_func_fcntl_f_dupfd_cloexec=yes \

It seems that gnulib has a fcntl.h that defines F_GETFD to 2, but make already has code to define F_GETFD to 1 and F_SETLKW to 2, so we get this compiler error:

../src/w32/compat/posixfcn.c: In function ‘fcntl’:
../src/w32/compat/posixfcn.c:50:7: error: duplicate case value
   50 |       case F_SETLKW:
      |       ^~~~
../src/w32/compat/posixfcn.c:43:7: note: previously used here
   43 |       case F_GETFD:
      |       ^~~~

When I leave fcntl as-is, I hit a different gnulib issue:

/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-win32/../../../../x86_64-w64-mingw32/lib/libmsvcrt.a(libmsvcrt-oss00292.o):(.text+0x0): multiple definition of `_isatty'; src/w32/compat/posixfcn.o:posixfcn.c:(.text+0x629): first defined here

Here the trouble is that a newer version of gnulib (i.e. newer than in the NDK source tree) adds a bunch of defines in unistd.h, like so:

#define read _read
#define open _open
#define isatty _isatty

make is defining its own isatty in src/w32/compat/posixfcn.c because the MSVC isatty returns true for the NUL file. With mingw-w64, io.h declares _isatty as dllimport, but isatty as an ordinary function. The result is that, even though main.c and posixfcn.c both have #define isatty _isatty, the reference to isatty in main.o refers to __imp__isatty, while posixfcn.o defines _isatty. libmsvcrt.a(libmsvcrt-oss00292.o) provides both __imp__isatty and _isatty.

There is a commit message suggesting that this redefining can be turned off, but I don't know how to do that.

mingw-w64 and MSVC both provide the unprefixed names AFAICT, but I see MSDN docs and header annotations indicating that the unprefixed names are deprecated.

I'm not sure how to make the gnulib autoconf stuff work, so my preference is to:

  • Just apply my patch locally, or
  • Switch to the upstream-supported Windows configuration (ie. build_w32.bat, using MSVC -- maybe using kokoro?), or
  • Just remove the output synchronization flag (-O) from ndk-build.cmd.

@DanAlbert
Copy link
Member

Switching to MSVC is usually a good idea. Might be more work than it's worth compared to just patching locally though.

@rprichard
Copy link
Collaborator

There is another variant of this bug that happens on macOS:

On macOS, the output-sync feature tries to lock the output file, but the file locking APIs don't work on a pipe. This problem started happening with NDK r21. Gradle captures the output of ndk-build (probably using a pipe), and I see the errors with either Android Studio or running ./gradlew at the CLI, but only if I configure gradle to pass -jN to ndk-build, which isn't the default.

It looks like the r21e macOS make output-sync works fine if the output is a tty or a real file, but not if it's a pipe.

@rprichard
Copy link
Collaborator

For Windows, I put in a workaround in r23 beta6 / rc1:
https://android-review.googlesource.com/c/platform/ndk/+/1755318/

That workaround isn't in the final r23 release, though.

Maybe it was lost during the transition from git_ndk-r23-release to aosp-ndk-release-r23?

I swapped out make's build system, which actually fixes this issue instead (but only on Windows, not macOS):

I suppose we want one of these fixes in r23b, but I'm not sure which.

@DanAlbert
Copy link
Member

First option (dropping -O) is the safest. We should probably start there, especially since it's what we intended to ship. Might end up doing the other in r23c depending on how the M1 work goes?

chenguoyin-alibaba pushed a commit to riscv-android-src/platform-ndk that referenced this issue Oct 21, 2021
This option was added in r21, but it was broken with the upgrade to
GNU Make 4.3. We can pass it again when make is fixed

Bug: android/ndk#1492
Test: manual
Change-Id: Idaddf69194ddf616249ff1671d35d3962f949d9c
(cherry picked from commit 0ecc433)
chenguoyin-alibaba pushed a commit to riscv-android-src/platform-ndk that referenced this issue Oct 21, 2021
This option was added in r21, but it was broken with the upgrade to
GNU Make 4.3. We can pass it again when make is fixed

Bug: android/ndk#1492
Test: manual
Change-Id: Idaddf69194ddf616249ff1671d35d3962f949d9c
(cherry picked from commit 0ecc433)
chenguoyin-alibaba pushed a commit to riscv-android-src/platform-ndk that referenced this issue Oct 21, 2021
This option was added in r21, but it was broken with the upgrade to
GNU Make 4.3. We can pass it again when make is fixed

Bug: android/ndk#1492
Test: manual
Change-Id: Idaddf69194ddf616249ff1671d35d3962f949d9c
(cherry picked from commit 0ecc433)
chenguoyin-alibaba pushed a commit to riscv-android-src/platform-ndk that referenced this issue Oct 21, 2021
Also create a category for all the CMake-related fixes in r23b.

Bug: android/ndk#1492
Test: treehugger
Change-Id: Ie74b1b42e1831fb0027a4d25c8750a6be9b5f252
(cherry picked from commit 2c7b92e)
MaoHan001 pushed a commit to riscv-android-src/platform-ndk that referenced this issue Jun 22, 2022
This change stops linking gnulib's fcntl on Windows, fixing --output-sync.

Bug: android/ndk#1492
Bug: android/ndk#1546
Change-Id: I8cd16eda2899451a6ae43ec1cf7008122746c576
(cherry picked from commit d5a5e29)
Merged-In: I8cd16eda2899451a6ae43ec1cf7008122746c576
@DanAlbert
Copy link
Member

We're well past r23 now :)

@rajatgupta1998
Copy link

Is this fixed? Not screaming that this is actually causing problems, but I see something similar on macOS as well.
Not sure if this is relevant either.

Environment details:

  • macOS ventura 13.4 - Apple Silicon M2 Pro
  • Android Studio Flamingo | 2022.2.1 Patch 2
    Build #AI-222.4459.24.2221.10121639, built on May 12, 2023.
    Runtime version: 17.0.6+0-17.0.6b802.4-9586694 aarch64
  • NDK version 25.2.9519653
  • Android SDK compile sdk and build tools 33.x
    Screenshot below:
    image

rooteduniverse1003 pushed a commit to rooteduniverse1003/Android-ToolChain-Make that referenced this issue Nov 21, 2023
For Windows, we need to avoid cross-compiling using the configure
script, because it will link with more gnulib modules than is
supported. gnulib's fcntl.c module breaks --output-sync (NDK bug 1492).
Instead, mimic build_w32.bat by:
 - building the same list of C files
 - copying the same gnulib headers
 - reusing the same config.h.W32.template file

For Darwin, the host-cpu-c-abi gnulib module breaks universal builds,
but it's easy to disable the module by editing config.h by hand
afterwards. (Removing that module from bootstrap.conf and rerunning
bootstrap might also be an option, but it changes a lot of stuff.)

I created CMakeLists.txt by translating Android.bp manually.

Bug: android/ndk#1492
Bug: android/ndk#1546
Change-Id: I367109233967ec6f0a1637b3ab262a234c93bc5b
(cherry picked from commit 487fc28)
Merged-In: I367109233967ec6f0a1637b3ab262a234c93bc5b
rooteduniverse1003 pushed a commit to rooteduniverse1003/Android-ToolChain-Make that referenced this issue Nov 21, 2023
For Windows, we need to avoid cross-compiling using the configure
script, because it will link with more gnulib modules than is
supported. gnulib's fcntl.c module breaks --output-sync (NDK bug 1492).
Instead, mimic build_w32.bat by:
 - building the same list of C files
 - copying the same gnulib headers
 - reusing the same config.h.W32.template file

For Darwin, the host-cpu-c-abi gnulib module breaks universal builds,
but it's easy to disable the module by editing config.h by hand
afterwards. (Removing that module from bootstrap.conf and rerunning
bootstrap might also be an option, but it changes a lot of stuff.)

I created CMakeLists.txt by translating Android.bp manually.

Bug: android/ndk#1492
Bug: android/ndk#1546
Change-Id: I367109233967ec6f0a1637b3ab262a234c93bc5b
(cherry picked from commit 487fc28)
Merged-In: I367109233967ec6f0a1637b3ab262a234c93bc5b
rooteduniverse1003 pushed a commit to rooteduniverse1003/Android-ToolChain-Make that referenced this issue Nov 21, 2023
For Windows, we need to avoid cross-compiling using the configure
script, because it will link with more gnulib modules than is
supported. gnulib's fcntl.c module breaks --output-sync (NDK bug 1492).
Instead, mimic build_w32.bat by:
 - building the same list of C files
 - copying the same gnulib headers
 - reusing the same config.h.W32.template file

For Darwin, the host-cpu-c-abi gnulib module breaks universal builds,
but it's easy to disable the module by editing config.h by hand
afterwards. (Removing that module from bootstrap.conf and rerunning
bootstrap might also be an option, but it changes a lot of stuff.)

I created CMakeLists.txt by translating Android.bp manually.

Bug: android/ndk#1492
Bug: android/ndk#1546
Change-Id: I367109233967ec6f0a1637b3ab262a234c93bc5b
(cherry picked from commit 487fc28)
Merged-In: I367109233967ec6f0a1637b3ab262a234c93bc5b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants