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

std.zig.system.NativeTargetInfo: improve examined file search #16973

Closed
wants to merge 1 commit into from
Closed

std.zig.system.NativeTargetInfo: improve examined file search #16973

wants to merge 1 commit into from

Conversation

BratishkaErik
Copy link
Contributor

@BratishkaErik BratishkaErik commented Aug 26, 2023

  • Append Termux-specific path to zig_examined_elf_files to workaround different path for env binary. See also Compiling a program on termux aarch64 produces FileNotFound errors #14146

  • Use same buffer for searching shebang line to search ELF magic constant, if any, and remove corresponding (now duplicate) check from abiAndDynamicLinkerFromFile

  • Fix not finding interpret path when it it separater from shebang by one or more blank spaces

  • Fix too big minimal limit of bytes from file (minimal shebang length instead of buffer.len)

BTW with this changes Zig correctly chases files from shebang line again, but abiAndDynamicLinkerFromFile returns incorrect result 2.19.0 instead of 2.37 (hidden bug popped up), so this PR is not enough to mitigate all related issues.

@BratishkaErik
Copy link
Contributor Author

I forgot to add, it is an alternative to #15896, mentioned in comment #15896 (comment)

@BratishkaErik
Copy link
Contributor Author

BratishkaErik commented Aug 30, 2023

As per discussion in Discord: environment variable was removed. Instead, Termux-specific path is hardcoded in that variable.
https://github.com/ziglang/zig/compare/acdc768849dc61a2e853f10a51a3f1d559daba71..afb3fb366414c7e400547a61c4ceb76dca85e874

-    const zig_examined_elf_files_envvar = std.os.getenv("ZIG_EXAMINED_ELF_FILES") orelse "/usr/bin/env";
-    var examined_elf_file_names_iterator = mem.tokenizeScalar(u8, zig_examined_elf_files_envvar, fs.path.delimiter);
+    const zig_examined_elf_files = std.fmt.comptimePrint("/usr/bin/env{[0]}/data/data/com.termux/files/usr/bin/env", .{fs.path.delimiter});
+    var examined_elf_file_names_iterator = mem.tokenizeScalar(u8, zig_examined_elf_files, fs.path.delimiter);

@kubkon kubkon requested a review from andrewrk September 4, 2023 07:43
@BratishkaErik
Copy link
Contributor Author

I can't reproduce errors at all...

* Append Termux-specific path to `zig_examined_elf_files` to workaround different path for `env` binary. See also #14146

* Use same buffer for searching shebang line to search ELF magic
  constant, if any, and remove corresponding (now duplicate) check from
  `abiAndDynamicLinkerFromFile`

* Fix not finding interpret path when it it separater from shebang by
  one or more blank spaces

* Fix too big minimal limit of bytes from file (minimal shebang length instead of `buffer.len`)

Signed-off-by: Eric Joldasov <bratishkaerik@getgoogleoff.me>
@BratishkaErik
Copy link
Contributor Author

I'll close it for a bit while kubkon upstreams his zld/elf linker. It's not blocking me, just that in my following commit (with fixing CI failure) I also deduplicated (and sometimes duplication is incomplete) std.elf logic in abi-parser functions here, and I don't know what will be changed in std.elf after upstreaming, so I'll probably need to redo deduplication work anyway.

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

Successfully merging this pull request may close these issues.

1 participant