-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
[K/N] Expose program name in runtime #5281
base: master
Are you sure you want to change the base?
Conversation
*^[KTI-54606](https://youtrack.jetbrains.com/issue/KT-54606) Fixed*
@@ -380,6 +380,10 @@ standaloneTest("throw_from_except_constr") { | |||
source = "runtime/exceptions/throw_from_except_constr.kt" | |||
} | |||
|
|||
standaloneTest("program_name") { | |||
source = "runtime/program_name/runtime_program_name.kt" | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file represents the legacy test infrastructure that we are actively trying to get rid of.
Please create a proper test class in
https://github.com/JetBrains/kotlin/tree/master/native/native.tests/tests/org/jetbrains/kotlin/konan/test/blackbox instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ported the testcase to the new infrastructure.
@@ -30,6 +30,8 @@ using namespace kotlin; | |||
//--- Setup args --------------------------------------------------------------// | |||
|
|||
OBJ_GETTER(setupArgs, int argc, const char** argv) { | |||
kotlin::programName = argv[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is argv[0]
guaranteed to live long enough? What will happen if some code accesses kotlin::programName
after the main
function finishes?
Is argv
guaranteed to always have at least one element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- programName will now be set to null before argv leaves scope. I could not find a definite answer if it would be guaranteed to live long enough, so I opted for the safe choice.
- Theoretically it is guaranteed by the POSIX standard. However if it has 0 arguments, the code 1 line below would have crashed. I also added a testcase for argc=0, and added support for it via
std::max(0, ...)
. So now kotlin executables don't crash on startup when they are launched in a non posix compatible way.
val programFileName = Platform.programName.substringAfterLast("/").substringBeforeLast(".") | ||
|
||
assertEquals("program_name", programFileName) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More tests are necessary. For example, the original issue mentions a nice use case:
Build multi-call binaries. For example, busybox is a single binary that contains many tools, and decides which tool to run based on how the symbolic link is called: https://busybox.net/downloads/BusyBox.html#usage (that's how it can be so small)
So checking a case with symbolic link would also be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added test for that by calling execv()
in the C code. This allowed to test even more use-cases (no programName at all), while still covering the use-case of renaming/linking a binary.
# Conflicts: # kotlin-native/backend.native/tests/build.gradle
native/native.tests/build.gradle.kts
Outdated
@@ -51,6 +51,7 @@ val stdlibK2Test = nativeTest("stdlibK2Test", "stdlib & frontend-fir") | |||
val kotlinTestLibraryTest = nativeTest("kotlinTestLibraryTest", "kotlin-test & !frontend-fir") | |||
val kotlinTestLibraryK2Test = nativeTest("kotlinTestLibraryK2Test", "kotlin-test & frontend-fir") | |||
val partialLinkageTest = nativeTest("partialLinkageTest", "partial-linkage") | |||
val programNameTest = nativeTest("programNameTest", "program-name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit redudant. I mean, running these tests separately shouldn't happen frequently, so adding a Gradle task is overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've removed it.
|
||
fun main(args: Array<String>) { | ||
// Remove path and extension (.kexe or .exe) | ||
val programFileName = Platform.programName?.substringAfterLast("/")?.substringBeforeLast(".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails on Windows, because it uses \
instead of /
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you. The sanitization is now platform agnostic.
val result = runProcess(cExecutable.absolutePath, binaryName, *args) { | ||
timeout = 60.seconds | ||
} | ||
assertEquals("calling exec...\n$expected", result.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this code should be fixed to handle \r\n
on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you again! The assertion is now platform agnostic due to additional sanitization.
|
||
fun validate(expected: String, vararg args: String) { | ||
val binaryName = kotlinCompilation.resultingArtifact.executableFile.path | ||
val result = runProcess(cExecutable.absolutePath, binaryName, *args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails if the test target is different from the host. You can reproduce this by running this test with -Pkotlin.internal.native.test.target=<target>
Gradle property.
Please use a proper executor through
Line 18 in 8c69667
val Settings.executor: Executor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this insight. I changed it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fails on linuxArm64 target with:
java.lang.AssertionError: Expected <calling exec...
programName: app
args:>, actual <calling exec...
exec failed>.
at kotlin.test.DefaultAsserter.fail(DefaultAsserter.kt:16)
at kotlin.test.Asserter$DefaultImpls.assertTrue(Assertions.kt:694)
at kotlin.test.DefaultAsserter.assertTrue(DefaultAsserter.kt:11)
at kotlin.test.Asserter$DefaultImpls.assertEquals(Assertions.kt:713)
at kotlin.test.DefaultAsserter.assertEquals(DefaultAsserter.kt:11)
at kotlin.test.AssertionsKt__AssertionsKt.assertEquals(Assertions.kt:63)
at kotlin.test.AssertionsKt.assertEquals(Unknown Source)
at kotlin.test.AssertionsKt__AssertionsKt.assertEquals$default(Assertions.kt:62)
at kotlin.test.AssertionsKt.assertEquals$default(Unknown Source)
at org.jetbrains.kotlin.konan.test.blackbox.ProgramNameTest.programNameTest$validate(ProgramNameTest.kt:49)
at org.jetbrains.kotlin.konan.test.blackbox.ProgramNameTest.programNameTest(ProgramNameTest.kt:54)
...
To reproduce, you can run the test with -Pkotlin.internal.native.test.target=linuxArm64
Gradle property on a Linux/x86_64 machine. I don't have access to such a machine at the moment. Could you please check the test, if possible?
Our test infrastructure runs tests on linuxArm64
through qemu, maybe the problem is somehow related to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could reproduce the problem by running the test via ./gradlew :native:native.tests:test --tests "org.jetbrains.kotlin.konan.test.blackbox.ProgramNameTest" -Pkotlin.internal.native.test.target=linux_arm64
on an x86 linux machine. However you are right, the current test infrastructure with qemu user mode emulation seems to not handle the exec
syscall in the way we need it. I also tried upgrading qemu to version 9.0.0, but this also didn't resolve the issue. If I understand the following qemu-comment correctly, qemu can't exec
into a new process which runs again on qemu, but always does a syscall to the host system. This also explains why exec fails with error code 8 (Exec format error), as qemu would instruct the x86 host system to run the arm64 binary:
at the point of execve the process leaves QEMU's control
(qemu syscall.c)
I see now some ways to approach the problem:
A) Disable the testcase in case of testRunSettings.executor is EmulatorExecutable
. Note that this testcase already tests the handling of the "default" program name from the exec
syscall the JVM does for us. So we already know that reading a value from argv[0]
works. Then this PR could probably be merged when no new points come up.
B) Create a FullHostSystemEmulatorExecutable, which uses qemu system emulation instead of qemu user space emulation. I'm not sure if this wouldn't be way out of scope for this PR.
Do you have any other ideas? If not, are you fine with approach A?
--
I also managed to run the EmulatorExecutable with the following command, which just does qemu -> main.cexe -> qemu -> app.kexe: /.../qemu-aarch64 -L /.../sysroot /.../main.cexe /.../qemu-aarch64 qemu-aarch64 -L /.../sysroot /.../app.kexe
(minimal refactoring needed in EmulatorExecutable.kt). However, also in this case we can not control the exec
syscall that actually starts app.kexe
, as this is part of the qemu user mode emulation infrastrucutre. There qemu sets then manually argv[0]=app
when launching app.kexe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any other ideas? If not, are you fine with approach A?
No ideas. I absolutely fine with it. Moreover, it matches our approach to testing linuxArm64: https://kotlinlang.org/docs/native-target-support.html#tier-2
The target is regularly tested on CI to be able to compile, but may not be automatically tested to be able to run.
// No program name - this would not be POSIX compliant, see https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html: | ||
// "[...] requires a Strictly Conforming POSIX Application to pass at least one argument to the exec function" | ||
// However, we should not crash the Kotlin runtime because of this. | ||
validate("programName: null\nargs:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Linux, this fails for me with
java.lang.AssertionError: Expected <calling exec...
programName: null
args:>, actual <calling exec...
programName:
args:>.
Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very interesting. Linux behaves here differently as windows & macOS. I fixed the problem, and added more explanation in the code. I used the following C standard PDF: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf.
Please tell me if you would want to let windows+macOS behave like linux. The way I wrote it now feels more natural to me, however I fully understand that all 3 options have its benefits and drawbacks:
A) The way I wrote it now: all 3 platforms behave the same; no program name leads to programName=null
, and empty programName leads to programName=""
.
B) all 3 platforms behave the same; however both no program name and empty program Name lead to programName=""
. (the linux way of thinking)
C) Fully native behaviour: no programName will be programName=""
on linux and programName=null
on macOS/windows; and empty programName will be everywhere programName=""
.
@@ -8,6 +8,7 @@ | |||
|
|||
#include "Porting.h" | |||
#include "Memory.h" | |||
#include "KString.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
|
||
// Kotlin executable name is in argv[1] | ||
// Forward argv[2..n] to kotlin executable as arguments (the first one should be the programName according to posix) | ||
execv(argv[1], &(argv[2])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, execv
requires that the list of arguments must be terminated by a NULL pointer. Is it always guaranteed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argv
is an array of size argc + 1
and the last member is always NULL
: https://en.cppreference.com/w/c/language/main_function
|
||
fun validate(expected: String, vararg args: String) { | ||
val binaryName = kotlinCompilation.resultingArtifact.executableFile.path | ||
val result = runProcess(cExecutable.absolutePath, binaryName, *args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fails on linuxArm64 target with:
java.lang.AssertionError: Expected <calling exec...
programName: app
args:>, actual <calling exec...
exec failed>.
at kotlin.test.DefaultAsserter.fail(DefaultAsserter.kt:16)
at kotlin.test.Asserter$DefaultImpls.assertTrue(Assertions.kt:694)
at kotlin.test.DefaultAsserter.assertTrue(DefaultAsserter.kt:11)
at kotlin.test.Asserter$DefaultImpls.assertEquals(Assertions.kt:713)
at kotlin.test.DefaultAsserter.assertEquals(DefaultAsserter.kt:11)
at kotlin.test.AssertionsKt__AssertionsKt.assertEquals(Assertions.kt:63)
at kotlin.test.AssertionsKt.assertEquals(Unknown Source)
at kotlin.test.AssertionsKt__AssertionsKt.assertEquals$default(Assertions.kt:62)
at kotlin.test.AssertionsKt.assertEquals$default(Unknown Source)
at org.jetbrains.kotlin.konan.test.blackbox.ProgramNameTest.programNameTest$validate(ProgramNameTest.kt:49)
at org.jetbrains.kotlin.konan.test.blackbox.ProgramNameTest.programNameTest(ProgramNameTest.kt:54)
...
To reproduce, you can run the test with -Pkotlin.internal.native.test.target=linuxArm64
Gradle property on a Linux/x86_64 machine. I don't have access to such a machine at the moment. Could you please check the test, if possible?
Our test infrastructure runs tests on linuxArm64
through qemu, maybe the problem is somehow related to that.
* [null] if the Kotlin code was compiled to a native library and the executable is not a Kotlin program. | ||
*/ | ||
public val programName: String? | ||
get() = Platform_getProgramName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qurbonzoda could you please review the stdlib change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -30,8 +30,12 @@ using namespace kotlin; | |||
//--- Setup args --------------------------------------------------------------// | |||
|
|||
OBJ_GETTER(setupArgs, int argc, const char** argv) { | |||
if (argc > 0 && argv[0][0] != '\0') { | |||
kotlin::programName = argv[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about doing kotlin::programName = strndup(argv[0], 4096)
? No need to free
the result, and we don't have to worry about lifetimes. 4096
is just some random not-too-small not-too-big number to protect us from argv[0]
being too large. Technically, the OS has some limits already, but might as well protect ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I applied your idea.
@@ -30,8 +30,12 @@ using namespace kotlin; | |||
//--- Setup args --------------------------------------------------------------// | |||
|
|||
OBJ_GETTER(setupArgs, int argc, const char** argv) { | |||
if (argc > 0 && argv[0][0] != '\0') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a comment explaining the purpose of argv[0][0] != '\0'
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I also commented it here and not only indirectly in the testcase.
|
||
int main() { | ||
programName(); | ||
fflush(NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an explicit return 0;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done
execv(argv[1], &(argv[2])); | ||
|
||
printf("exec failed\n"); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return non-zero exit code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I also now print the errno
for easier debugging in case the exec
syscall fails.
This reverts commit fd2ebbf.
@@ -15,6 +15,7 @@ | |||
*/ | |||
|
|||
#include <cstdlib> | |||
#include <string.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps #include <cstring>
, to match the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not any more relevant, see comment below.
if (argc > 0 && argv[0][0] != '\0') { | ||
// Don't set the programName to an empty string (by checking argv[0][0] != '\0') to make all platforms behave the same: | ||
// Linux would set argv[0] to "" in case no programName is passed, whereas Windows & macOS would set argc to 0. | ||
kotlin::programName = strndup(argv[0], 4096); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny thing: this code doesn't compile on mingw_x64. You can observe this by running
./gradlew clangFrontendLauncherMainMingw_x64
Apparently strndup
is not available in (our version of) MinGW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats really interesting. I therefore reverted to the old kotlin::programName = argv[0];
implementation. Those tasks compile then successfully: ./gradlew clangFrontendLauncherMainMingw_x64 clangFrontendLauncherMainAndroid_arm64 clangFrontendLauncherMainMacos_x64 clangFrontendLauncherMainLinux_x64
.
For some reason, the test fails on Windows with
|
^KTI-54606 Fixed