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

Fix argument handling #578

Merged
merged 11 commits into from
Jun 30, 2022
Merged

Fix argument handling #578

merged 11 commits into from
Jun 30, 2022

Conversation

angelhof
Copy link
Member

Working to fix issue: #570

Signed-off-by: Konstantinos Kallas [email protected]

angelhof added 2 commits June 18, 2022 15:06
Signed-off-by: Konstantinos Kallas <[email protected]>
Signed-off-by: Konstantinos Kallas <[email protected]>
@mgree
Copy link
Collaborator

mgree commented Jun 22, 2022

Pinging: if you can post a summary before hacking next week, I can promise to show up having thought about it. 😁

@angelhof
Copy link
Member Author

The goal is to correctly store input arguments separately (and not join them and resplit them), so that we can keep spaces between them. In addition, we want to restore the arguments without expanding them (so that things like * are not expanded to the files in the directory).

I added a test file called test-star.sh (in evaluation/tests/interface_tests) with this commit (788b952) that can observe this issue if called with relevant arguments. For example:

$PASH_TOP/pa.sh test-star.sh  foo '*' baz "hi there Michael"

To resolve this, I came up with a partial solution that uses bash arrays to store the arguments. This would work out of the box if our unparser supported bash syntax, but since it does not, we have to do tricks like putting the bashism in a separate script and sourcing it (since the preprocessor can unparse a script that sources) (see https://github.com/binpash/pash/pull/578/files#diff-9117e6346fe0f0114b82710c6f433eccaeb3e07be02b26f00013024b1400af5a). This makes the preprocessed script look like this:

{ pash_previous_exit_status="${?}" ; }
 { { source /home/konstantinos/University/research/dish/runtime/save_args.sh "${@}" ; }
 { { pash_disable_parallel_pipelines=0 ; }
 { { source /home/konstantinos/University/research/dish/compiler/pash_runtime.sh /tmp/pash_iQnfOVm/tmpaf611ke8 /tmp/pash_iQnfOVm/tmpjqq8fmc4 --graphviz no --graphviz_dir /tmp --r_split_batch_size 1000000 --debug 1 --termination clean_up_graph --speculation no_spec --width 2 ; }
 { { set -- $(source /home/konstantinos/University/research/dish/runtime/restore_args.sh) ; }
 { ( exit "${pash_runtime_final_status}" ) ; } ; } ; } ; } ; }
{ pash_previous_exit_status="${?}" ; }
 { { source /home/konstantinos/University/research/dish/runtime/save_args.sh "${@}" ; }
 { { pash_disable_parallel_pipelines=1 ; }
 { { source /home/konstantinos/University/research/dish/compiler/pash_runtime.sh /tmp/pash_iQnfOVm/tmpqvb00t8a /tmp/pash_iQnfOVm/tmp1tey1ouu --graphviz no --graphviz_dir /tmp --r_split_batch_size 1000000 --debug 1 --termination clean_up_graph --speculation no_spec --width 2 ; }
 { { set -- $(source /home/konstantinos/University/research/dish/runtime/restore_args.sh) ; }
 { ( exit "${pash_runtime_final_status}" ) ; } ; } ; } ; } ; }

while in the pash_wrap_vars.sh, I just perform the storing/restoring normally:

## Store
export pash_input_args=( "$@" )

## Restore
set -- "${pash_input_args[@]}"

Unfortunately, I can't find a way to restore using a separate script, i.e., restore_args.sh. The best I got was to get it to print them, but this unfortunately ends up adding extra quotes and still splitting different arguments, as follows:

$PASH_TOP/pa.sh test-star.sh  foo '*' baz "hi there Michael"
foo
*
baz
hi there Michael
"foo"
"*"
"baz"
"hi
there
Michael"

This shows that the first storing (in preprocessed script) and the first restoring (in pash_wrap_vars.sh) work. We just have to get the restoration in the preprocessed script to work too.

Some ideas are:

  • Can we get the parser to produce the correct restoration somehow, i.e., set -- "${pash_input_args[@]}"?
  • Could we somehow use eval and just produce a string that eval would run?
  • Could we correctly print the arguments in restore_args.sh so that we can correctly unparse them after the command substitution?

@angelhof angelhof changed the title [WIP] start fixing argument handling Fix argument handling Jun 27, 2022
@angelhof angelhof marked this pull request as ready for review June 27, 2022 20:14
@angelhof
Copy link
Member Author

@mgree I think this is done, we should review it on Thursday and merge it!

@angelhof angelhof requested a review from mgree June 27, 2022 20:15
@github-actions
Copy link

OS:ubuntu-20.04
Mon Jun 27 20:23:03 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 36/54 tests passed.
agg: 109/109 tests passed.
diff.sh are not identical
diff.sh are not identical
set-diff.sh are not identical
set-diff.sh are not identical
export_var_script.sh are not identical
export_var_script.sh are not identical
comm-par-test.sh are not identical
comm-par-test.sh are not identical
comm-par-test2.sh are not identical
comm-par-test2.sh are not identical
tee_web_index_bug.sh are not identical
tee_web_index_bug.sh are not identical
fun-def.sh are not identical
fun-def.sh are not identical
bigrams.sh are not identical
bigrams.sh are not identical
for_loop_simple.sh are not identical
for_loop_simple.sh are not identical

@github-actions
Copy link

OS = Debian 10
CPU = Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
Ram = 15752
Hash = ee51665
Kernel= Linux 4.15.0-167-generic x86_64

benchmark tests passed failed untested unresolved unsupported not_in_use other_status
posix 494 375 41 31 6 40 1 0
intro 2 2 0 0 0 0 0 0
interface 39 39 0 0 0 0 0 0
compiler 54 36 18 0 0 0 0 0
aggregator 109 108 1 0 0 0 0 0

@github-actions
Copy link

OS:ubuntu-20.04
Mon Jun 27 22:40:10 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 54/54 tests passed.
agg: 109/109 tests passed.

@github-actions
Copy link

OS:ubuntu-18.04
Mon Jun 27 22:41:32 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 54/54 tests passed.
agg: 109/109 tests passed.

@github-actions
Copy link

OS = Debian 10
CPU = Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
Ram = 15752
Hash = fbbc6e6
Kernel= Linux 4.15.0-167-generic x86_64

benchmark tests passed failed untested unresolved unsupported not_in_use other_status
posix 494 375 41 31 6 40 1 0
intro 2 2 0 0 0 0 0 0
interface 39 39 0 0 0 0 0 0
compiler 54 54 0 0 0 0 0 0
aggregator 109 108 1 0 0 0 0 0

@angelhof
Copy link
Member Author

FYI, the solution involves:

  • bash arrays to store arguments (instead of joining/splitting arguments ourselves)
  • extensions to our custom expansion to properly parse bash arrays from a declared variables file, and extensions to correctly expand arguments

Signed-off-by: Konstantinos Kallas <[email protected]>
@github-actions
Copy link

OS:ubuntu-18.04
Thu Jun 30 14:41:22 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 54/54 tests passed.
agg: 109/109 tests passed.

@github-actions
Copy link

OS:ubuntu-20.04
Thu Jun 30 14:43:11 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 54/54 tests passed.
agg: 109/109 tests passed.

@github-actions
Copy link

OS = Debian 10
CPU = Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
Ram = 15752
Hash = c0ae389
Kernel= Linux 4.15.0-167-generic x86_64

benchmark tests passed failed untested unresolved unsupported not_in_use other_status
posix 494 375 41 31 6 40 1 0
intro 2 2 0 0 0 0 0 0
interface 39 39 0 0 0 0 0 0
compiler 54 54 0 0 0 0 0 0
aggregator 109 108 1 0 0 0 0 0

@angelhof angelhof merged commit 7cd4c1c into future Jun 30, 2022
@angelhof angelhof deleted the arg-handling branch June 30, 2022 15:05
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.

2 participants