Force resolution of fstat64 symbol with JNA #110807
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When JNA loads libraries it creates a proxy object for the library. Unfortunately it doesn't actually inspect any of the methods, those get bound lazily at runtime when the method is called through the proxy. For fstat64 we need to know at load time whether the symbol exists, so that we can fallback to an alternate function if it doesn't. This commit looks up the NativeLibrary object from JNA for libc and checks if fstat64 exists during load time.
elasticsearchmachine
added
v8.16.0
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
prdoyle
approved these changes
Jul 12, 2024
| fstat64 = Native.load("c", FStat64Function.class); | ||
| } catch (UnsatisfiedLinkError e) { | ||
| // TODO: explain | ||
| // fstat has a long history in linux from the 32-bit architecture days. On some modern linux systems, |
There was a problem hiding this comment.
Does this make the class name JnaPosixCLibrary a bit of a misnomer?
There was a problem hiding this comment.
Maybe, but "fstat" is part of posix, it's just that the implementation is a bit wonky due to historical reasons (and thus why we need to sometimes peek into the implementation side).
| // fstat has a long history in linux from the 32-bit architecture days. On some modern linux systems, | ||
| // fstat64 doesn't exist as a symbol in glibc. Instead, the compiler replaces fstat64 calls with | ||
| // the internal __fxstat method. Here we fall back to __fxstat, and staticall bind the special | ||
| // "version" argument so that the call site looks the same as that of fstat64 | ||
| var fxstat = Native.load("c", FXStatFunction.class); | ||
| int version = System.getProperty("os.arch").equals("aarch64") ? 0 : 1; |
There was a problem hiding this comment.
Hmm this seems worth documenting.
There was a problem hiding this comment.
That's what I was doing here with the comment. Do you see a better place to document it?
This was referenced Jul 13, 2024
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jul 13, 2024
Native preallocation has several issues, introduced in a refactoring for 8.13. First, the native allocator is never even tried, it always decides to fall back to the Java setLength method. Second, the stat method did not work correctly on all systems, see elastic#110807. This commit fixes native preallocate to properly execute on Linux, as well as MacOS. It also adds direct tests of preallocation. Note that this is meant as a bugfix for 8.15, so as minimal a change as possible is made here. The code has completely changed in main. Some things like the new test and fixes for macos will be forward ported to main, but I did not want to make larger changes in a bugfix.
rjernst
added a commit
that referenced
this pull request
Jul 15, 2024
Native preallocation has several issues, introduced in a refactoring for 8.13. First, the native allocator is never even tried, it always decides to fall back to the Java setLength method. Second, the stat method did not work correctly on all systems, see #110807. This commit fixes native preallocate to properly execute on Linux, as well as MacOS. It also adds direct tests of preallocation. Note that this is meant as a bugfix for 8.15, so as minimal a change as possible is made here. The code has completely changed in main. Some things like the new test and fixes for macos will be forward ported to main, but I did not want to make larger changes in a bugfix.
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Oct 24, 2024
The test issue was fixed by elastic#110807 closes elastic#110801
elasticsearchmachine
pushed a commit
that referenced
this pull request
Oct 24, 2024
georgewallace
pushed a commit
to georgewallace/elasticsearch
that referenced
this pull request
Oct 25, 2024
The test issue was fixed by elastic#110807 closes elastic#110801
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Nov 4, 2024
The test issue was fixed by elastic#110807 closes elastic#110801
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When JNA loads libraries it creates a proxy object for the library. Unfortunately it doesn't actually inspect any of the methods, those get bound lazily at runtime when the method is called through the proxy. For fstat64 we need to know at load time whether the symbol exists, so that we can fallback to an alternate function if it doesn't.
This commit looks up the NativeLibrary object from JNA for libc and checks if fstat64 exists during load time.