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

[K/N] Expose program name in runtime #5281

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

vonox7
Copy link
Contributor

@vonox7 vonox7 commented Mar 21, 2024

^KTI-54606 Fixed

@ivakub ivakub added the Native label Apr 8, 2024
@@ -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"
}
Copy link
Contributor

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.

Copy link
Contributor Author

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];
Copy link
Contributor

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?

Copy link
Contributor Author

@vonox7 vonox7 Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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)
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@@ -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")
Copy link
Contributor

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.

Copy link
Contributor Author

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(".")
Copy link
Contributor

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 /.

Copy link
Contributor Author

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

// 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:")
Copy link
Contributor

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.

Copy link
Contributor Author

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unused.


// 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]));
Copy link
Contributor

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?


fun validate(expected: String, vararg args: String) {
val binaryName = kotlinCompilation.resultingArtifact.executableFile.path
val result = runProcess(cExecutable.absolutePath, binaryName, *args) {
Copy link
Contributor

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()
Copy link
Contributor

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?

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