cc-wrapper: simplify expandResponseParams parser by orivej · Pull Request #3 · obsidiansystems/nixpkgs

orivej · 2017-07-01T00:34:47Z

Import from https://github.com/orivej/expand-compiler-args/tree/ca9865a89eabee6282ed756a371825780f11fc25

ryantrinkle · 2017-07-02T16:03:58Z

Looks very reasonable to me, and I like that you handle the whole argument list rather than one at a time - eliminating the need to loop over it in bash.

I think it'd be a good idea to comment some of the trickier parts of this, in case anyone needs to come through this part of the codebase and fix or add functionality in the future.

orivej · 2017-07-02T17:45:37Z

I have added a docstring for expandArg that explains some decisions (to write NULL-terminated strings, to parse like GCC) and an assumption (that arg is NULL-terminated).

Ericson2314

I like the basic idea here a lot, good work! Agreed, on commenting the hell out of everything. Otherwise, think I found a bug, and a few style nits/questions. Otherwise good.

Ericson2314 · 2017-07-02T17:55:52Z

pkgs/build-support/cc-wrapper/expand-response-params.c

Can we use a State transitions[7][5] = [ [ .. ] .. ]? I think that would be cleaner.

Ericson2314 · 2017-07-02T18:02:24Z

pkgs/build-support/cc-wrapper/utils.sh

Unlike @ryantrinkle, I'm a bit less exited about forking regardless of whether there is a response file. But I can't think of any problems either, correctness-wise or bootstrapping-wise (it took me a second to realize that second part). I don't care to much, and you two outvote me, so lets keep it.

OTOH, great job with readarray and null deliminators. Huge fan of those :).

I have changed this to only call expand-response-params when @FILE args are present.

Thank you!

Ericson2314 · 2017-07-02T18:12:29Z

pkgs/build-support/cc-wrapper/expand-response-params.c

nit: EXIT_SUCCESS instead of 0.

OK, but I wouldn't use EXIT_SUCCESS in my code unless I would also use EXIT_FAILURE.

Ericson2314 · 2017-07-02T18:30:38Z

pkgs/build-support/cc-wrapper/expand-response-params.c

If argv[1] == 0 we'll free(0)

This is correct, free is defined to do nothing on NULL.

Oh! I did not know that.

Ericson2314 · 2017-07-02T18:32:47Z

pkgs/build-support/cc-wrapper/expand-response-params.c

What does binding s achieve?

append needs char *, whereas &c has type int * (because c needs to contain EOF which is not a char). It is also not correct to cast int * to char * because int * may point to a zero byte in the binary representation of the integer instead of the byte with the char.

Ah! Thank you that explains it. (Would be good for a comment though... :))

I'd rather add appendChar function than a comment about char s = c.

Ericson2314

I'll let Ryan do the merge / comments galore as he pointed out would still be nice, but looks great re all my points.

I was also trying to think of one way to make the initial state explicit in the transition table, but completely forgot about C99 sparse array initialization syntax. Great trick!

Ericson2314 · 2017-07-02T20:17:56Z

pkgs/build-support/cc-wrapper/expand-response-params.c

Oh! I did not know that.

Ericson2314 · 2017-07-02T20:27:55Z

pkgs/build-support/cc-wrapper/expand-response-params.c

I don't think the explicit values are needed anymore?

I could remove the numbers, but I still depend on their correspondence with transition columns, and C does not allow me to remove 5 from transitions[][5].

Ah the columns, duh. Nevermind then!

Ericson2314 · 2017-07-02T20:29:14Z

pkgs/build-support/cc-wrapper/default.nix

Hmm, I suppose both the old and new version should pass a -std=c99/-std=c11? Don't want some weirdness if somebody is bootstrapping with an impure stdenv.

Done. BTW, I still don't understand why src is copied.

Free free to try it with $src and investigate the error on Darwin! __FILE__ is only a guess.

src was copied for my version because otherwise there would be a spurious reference in the binary to the source, which would be picked up by Nix. I'm pretty sure that's because of my use of assert, which probably uses __FILE__ as @Ericson2314 mentioned. If it doesn't cause trouble with yours, I see no reason to keep the cp around.

What should I look for to recognize if there is a spurious reference or not? Does Nix refuse to install a derivation, or print a warning?

It should fail with an error if there's a reference that isn't in allowedReferences/allowedRequisites. However, you might also just grep for /nix/store in the resulting binaries.

orivej · 2017-07-02T23:06:13Z

I have just pushed an update that modified pkgs/stdenv/darwin/default.nix to invoke bash from bootstrap-tools as bash rather than as sh; otherwise bash disables its process substitution feature and does not accept < <("@expand-response-params@" "$@")

ryantrinkle · 2017-07-03T20:56:34Z

pkgs/build-support/cc-wrapper/expand-response-params.c

Could this be char * instead of void *?

Done. I used void * because it is the interface of memcpy, but char * makes more sense for strings.

ryantrinkle · 2017-07-03T20:59:19Z

pkgs/build-support/cc-wrapper/expand-response-params.c

Awesome! This is much easier for me to read.

Import from https://github.com/orivej/expand-compiler-args/tree/b2446902fe7816f34c3f21d29a66da3ee2b1601e

orivej · 2017-07-03T22:02:52Z

If you strongly believe that some lines will benefit from additional comments, please point them out. Otherwise, this pull request seems ready for merge.

ryantrinkle · 2017-07-03T22:03:34Z

@orivej Agreed! I'm doing some final testing now, and I'll merge ASAP.

ryantrinkle · 2017-07-04T02:39:52Z

@orivej Unfortunately, this implementation seems to have a bug that prevents GHC from building. Specifically, when escaped quotes (of any kind) are inside of quotes (of any kind), they do not produce output. For example, this response file produces no output:
'\''"\""'\"'"\'"

However, it should produce a single argument with the value:
'""'

Perhaps we should add some tests?

ryantrinkle · 2017-07-04T03:18:05Z

@orivej Could you take a look at my fix for that issue and let me know what you think?

orivej · 2017-07-04T12:52:22Z

Thank you, this is what I meant the code to do!

… crash 1. Gnumeric has unbalanced XML tags in its doc translations. 2. itstool's XML error handler tries to print this error with context. 3. libxml2's context snipper treats the data as bytes, not UTF-8. 4. python3Packages.libxml2 casts the context to a UTF-8 Python string. 5. itstool dereferences a null pointer. This patch intervenes at #4. In https://bugzilla.gnome.org/show_bug.cgi?id=789714#c4 , upstream suggests that intervening at #3 would be better -- that each of the four copies of xmlParserPrintFileContextInternal() have four additional UTF-8 problems, one of which is that the caret indicator ought to count "unicode characters" not bytes. But to position a caret correctly, a character count is not sufficient -- this would need to use icu's BiDi logic (with fallback to doing something wrong when libxml2 is configured not to use icu) -- which makes a 'correct' fix a much larger project than this simple band-aid.

Without this change it segfaults when trying to play any media: $ jellyfinmediaplayer Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway. libpng warning: iCCP: known incorrect sRGB profile Logging to /home/bf/.local/share/jellyfinmediaplayer/logs/jellyfinmediaplayer.log Cannot load libcuda.so.1 Segmentation fault (core dumped) The backtrace shows pipewire being at fault: $ coredumpctl debug [...] Program terminated with signal SIGSEGV, Segmentation fault. #0 0x00007f711428c9bb in core_event_demarshal_remove_id () from /nix/store/nhffrd7f15dhfbkwzgayq7hhzmdvdy19-pipewire-0.3.63-lib/lib/pipewire-0.3/libpipewire-module-protocol-native.so [Current thread is 1 (Thread 0x7f6ffdc87640 (LWP 1360949))] (gdb) bt #0 0x00007f711428c9bb in core_event_demarshal_remove_id () from /nix/store/nhffrd7f15dhfbkwzgayq7hhzmdvdy19-pipewire-0.3.63-lib/lib/pipewire-0.3/libpipewire-module-protocol-native.so #1 0x00007f711428886c in process_remote () from /nix/store/nhffrd7f15dhfbkwzgayq7hhzmdvdy19-pipewire-0.3.63-lib/lib/pipewire-0.3/libpipewire-module-protocol-native.so #2 0x00007f7114288e68 in on_remote_data () from /nix/store/nhffrd7f15dhfbkwzgayq7hhzmdvdy19-pipewire-0.3.63-lib/lib/pipewire-0.3/libpipewire-module-protocol-native.so #3 0x00007f7114310efe in loop_iterate () from /nix/store/nhffrd7f15dhfbkwzgayq7hhzmdvdy19-pipewire-0.3.63-lib/lib/spa-0.2/support/libspa-support.so #4 0x00007f71266fe7f2 in do_loop () from /nix/store/nhffrd7f15dhfbkwzgayq7hhzmdvdy19-pipewire-0.3.63-lib/lib/libpipewire-0.3.so.0 #5 0x00007f7128b08e86 in start_thread () from /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6 #6 0x00007f7128b8fce0 in clone3 () from /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6 (gdb) Standalone mpv doesn't segfault (when directly playing the underlying media files). I don't know why. Fixes: b97cda7 ("mpv-unwrapped: 0.34.1 -> 0.35.0") Fixes NixOS#205141 Ref jellyfin/jellyfin-desktop#341 (cherry picked from commit 3c528bc)

On my systems there is a path in the fonts list that looks like /nix/store/<hash>-<font>.ttf. The previous code assumed that every font path contains at least one more slash which isn't the case here. The code matched on the / after the $out path of the derivation: /nix/store/<hash>-<name>/share/X11/fonts/... ^ That slash only exists if the font lives in a subdirectory, which isn't strictly required. In my case I've a few font files that are located at /nix/store/<hash>-<name>.ttf and are assembled into a folder as symlinks. (See example below) Any attempt at calling realpath(3) on them (potentially recursively) will result in the single file entry in the store being returned. Whenever that font is being loaded flatpak segfaulted like so: ``` (gdb) bt #0 0x00005583b3673763 in get_nix_closure () #1 0x00005583b3673711 in get_nix_closure () #2 0x00005583b36737dc in add_nix_store_symlink_targets () #3 0x00005583b36749a8 in add_font_path_args () #4 0x00005583b367b3f4 in flatpak_run_app () #5 0x00005583b35e4f08 in flatpak_builtin_run () #6 0x00005583b35a5970 in main () (gdb) x/5i $rip => 0x5583b3673763 <get_nix_closure+307>: movb $0x0,(%rax) 0x5583b3673766 <get_nix_closure+310>: call 0x5583b35a46a0 <g_hash_table_add@plt> 0x5583b367376b <get_nix_closure+315>: jmp 0x5583b3673699 <get_nix_closure+105> 0x5583b3673770 <get_nix_closure+320>: xor %edi,%edi 0x5583b3673772 <get_nix_closure+322>: lea 0x30475(%rip),%rsi # 0x5583b36a3bee (gdb) x/s $rsi 0x5583e92d2180: "/nix/store/<hash>-A.ttf" (gdb) print $rax $1 = 0 ``` RAX pointing to 0 here corresponds to the pointer `p` in the source code. So the code attempted to dereference a null pointer as no further slash (the return value of strchr(3)) could be found. The modification in this commit ensures that we check for `p != 0` so we don't run into this situation again. Adding the path to the closure will still be done, even if no further slash can be found, as that still points to a valid store path. ---- Nix code for custom fonts: ```nix let files = [ ./A.ttf ./B.ttf ]; in runCommandNoCC "custom-fonts" {} '' mkdir -p $out/share/fonts/truetype cd $out/share/fonts/truetype ${ lib.concatStringsSep "\n" (map (file: "ln -s ${file} ${baseNameOf file}") files)} '' ```

orivej force-pushed the response-file-parsing-speed branch from 0db0650 to 9ed1ef5 Compare July 1, 2017 00:44

orivej mentioned this pull request Jul 1, 2017

cc-wrapper: improve response file parsing speed NixOS/nixpkgs#26974

Merged

8 tasks

orivej force-pushed the response-file-parsing-speed branch 6 times, most recently from c41edf5 to ba2757e Compare July 2, 2017 05:34

orivej force-pushed the response-file-parsing-speed branch from ba2757e to bf403ee Compare July 2, 2017 17:40

Ericson2314 requested changes Jul 2, 2017

View reviewed changes

orivej force-pushed the response-file-parsing-speed branch from bf403ee to 4e3021a Compare July 2, 2017 19:21

Ericson2314 approved these changes Jul 2, 2017

View reviewed changes

pkgs/build-support/cc-wrapper/expand-response-params.c Outdated

Copy link

Member

Ericson2314 Jul 2, 2017

Choose a reason for hiding this comment

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

Oh! I did not know that.

Ericson2314 reviewed Jul 2, 2017

View reviewed changes

orivej force-pushed the response-file-parsing-speed branch 2 times, most recently from 195c27b to 3def2b8 Compare July 2, 2017 23:01

orivej force-pushed the response-file-parsing-speed branch 2 times, most recently from 9995856 to f2ecf4a Compare July 3, 2017 11:28

ryantrinkle reviewed Jul 3, 2017

View reviewed changes

orivej force-pushed the response-file-parsing-speed branch from f2ecf4a to d7593dd Compare July 3, 2017 21:49

cc-wrapper: simplify expandResponseParams parser

2bc7b4e

Import from https://github.com/orivej/expand-compiler-args/tree/b2446902fe7816f34c3f21d29a66da3ee2b1601e

orivej force-pushed the response-file-parsing-speed branch from d7593dd to 2bc7b4e Compare July 3, 2017 21:51

ryantrinkle merged commit 2bc7b4e into obsidiansystems:response-file-parsing-speed Jul 4, 2017

orivej deleted the response-file-parsing-speed branch July 6, 2017 17:05

Conversation

orivej commented Jul 1, 2017

Uh oh!

ryantrinkle commented Jul 2, 2017

Uh oh!

orivej commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orivej Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orivej Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orivej commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orivej commented Jul 3, 2017

Uh oh!

ryantrinkle commented Jul 3, 2017

Uh oh!

ryantrinkle commented Jul 4, 2017

Uh oh!

ryantrinkle commented Jul 4, 2017

orivej commented Jul 2, 2017 •

edited

Loading

Ericson2314 left a comment •

edited

Loading

orivej Jul 2, 2017 •

edited

Loading

orivej Jul 2, 2017 •

edited

Loading