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

Unify shell assignment #4451

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

treseco
Copy link

@treseco treseco commented Apr 20, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

This change is in response to (#2129) and changes the behavior of the the e and aezv commands to match the existing assignment behavior of the ar & dr commands.

The changes related to e:

  • Old behavior was e k=v k=v ... allowing multiple assignments and no spaces around the '=' character.
  • New behavior is e k = v or e k=v.
  • e command description updated to new help message and arguments.
  • e command handler updated to new argument format.
  • Updated a test using e's help message to reflect the new output.

The changes to aezv:

  • Old behavior was aezv k v with two args for assignment, and no '=' character.
  • New behavior is aezv k = v or aezv k=v, and functionality of aezv k is preserved.
  • aezv command description updated to new help message and arguments.
  • aezv command handler updated to new argument format.
  • Updated a couple tests using aezv for assignment to have the '=' character.

The rational for selecting the ar/dr assignment form over the e form (as discussed in #2129) is that multiple assignments of config variables can still be accomplished in a single line via e k=v; e k=v as well as the alias and macro features. In addition, the aezv, ar, and dr commands support other operations than just assignment so something like ar rax=0x1 xmm PC could be valid, but is not as intuitive. Finally, allowing spaces around the '=' makes the shell a bit less picky.

This PR does not address the assignment used by $ to assign aliases. To do so would require additional clarification as $ also supports :=, -= and += for various features.
...

Test plan

Testing involves ensuring correct functionality of the e and aezv commands.

user@f77bf859cbe9:~/host/rizin$ rizin /bin/ls
 -- Use 'rz-bin -ris' to get the import/export symbols of any binary.
[0x000061d0]> e asm.lines
true
[0x000061d0]> e asm.lines = false
[0x000061d0]> e asm.lines
false
[0x000061d0]> e asm.lines=true
[0x000061d0]> e asm.lines
true
[0x000061d0]> e asm.lines = AAAAAAAaAA   aAAAAAAAAAAAAAA AAAAAAa
[0x000061d0]> e asm.lines
false
[0x000061d0]> e asm.lines = AAAAAAAaAA   aAAAAAAAAAAAAAA AAAAAAa
[0x000061d0]> e asm.lines
false
[0x000061d0]> e log.file=/dev/null log.level=5
[0x000061d0]> e log.file
/dev/null log.level=5
[0x000061d0]> aezi
[0x000061d0]> aezv
 PC: 0x00000000000061d0 cr0: 0x0000000000000000 fs: 0x0000000000000000 frip: 0x0000000000000000 rsp: 0x0000000000000000
 rbx: 0x0000000000000000 rdx: 0x0000000000000000 ftw: 0x0000 ss: 0x0000000000000000 af: false swd: 0x0000
 frdp: 0x0000000000000000 ac: false zf: false es: 0x0000000000000000 rdi: 0x0000000000000000 sf: false
 r14: 0x0000000000000000 r15: 0x0000000000000000 cf: false r12: 0x0000000000000000 r13: 0x0000000000000000
 r10: 0x0000000000000000 r11: 0x0000000000000000 tf: false dr0: 0x0000000000000000 gs: 0x0000000000000000
 fop: 0x0000 df: false nt: false rcx: 0x0000000000000000 pf: false if: false r9: 0x0000000000000000
 r8: 0x0000000000000000 ds: 0x0000000000000000 cs: 0x0000000000000000 st2: 0x00000000000000000000
 of: false cwd: 0x0000 st3: 0x00000000000000000000 st0: 0x00000000000000000000 rf: false st1: 0x00000000000000000000
 st6: 0x00000000000000000000 rax: 0x0000000000000000 st7: 0x00000000000000000000 vm: false st4: 0x00000000000000000000
 st5: 0x00000000000000000000 rsi: 0x0000000000000000 rbp: 0x0000000000000000
[0x000061d0]> aezv rbx
 rbx: 0x0000000000000000
[0x000061d0]> aezv rbx=0xffff
rbx = 0xffff
[0x000061d0]> aezv rbx
 rbx: 0x000000000000ffff
[0x000061d0]> aezv rbx = 0xffff0000
rbx = 0xffff0000
[0x000061d0]> aezv rbx
 rbx: 0x00000000ffff0000
[0x000061d0]> aezv rbx = rbx = rbx = PC
rbx = 0x61d0
[0x000061d0]> aezv
 PC: 0x00000000000061d0 cr0: 0x0000000000000000 fs: 0x0000000000000000 frip: 0x0000000000000000 rsp: 0x0000000000000000
 rbx: 0x00000000000061d0 rdx: 0x0000000000000000 ftw: 0x0000 ss: 0x0000000000000000 af: false swd: 0x0000
 frdp: 0x0000000000000000 ac: false zf: false es: 0x0000000000000000 rdi: 0x0000000000000000 sf: false
 r14: 0x0000000000000000 r15: 0x0000000000000000 cf: false r12: 0x0000000000000000 r13: 0x0000000000000000
 r10: 0x0000000000000000 r11: 0x0000000000000000 tf: false dr0: 0x0000000000000000 gs: 0x0000000000000000
 fop: 0x0000 df: false nt: false rcx: 0x0000000000000000 pf: false if: false r9: 0x0000000000000000
 r8: 0x0000000000000000 ds: 0x0000000000000000 cs: 0x0000000000000000 st2: 0x00000000000000000000
 of: false cwd: 0x0000 st3: 0x00000000000000000000 st0: 0x00000000000000000000 rf: false st1: 0x00000000000000000000
 st6: 0x00000000000000000000 rax: 0x0000000000000000 st7: 0x00000000000000000000 vm: false st4: 0x00000000000000000000
 st5: 0x00000000000000000000 rsi: 0x0000000000000000 rbp: 0x0000000000000000
[0x000061d0]> aezv rbx = PC = 0x0
rbx = 0x0
[0x000061d0]> aezv
 PC: 0x00000000000061d0 cr0: 0x0000000000000000 fs: 0x0000000000000000 frip: 0x0000000000000000 rsp: 0x0000000000000000
 rbx: 0x0000000000000000 rdx: 0x0000000000000000 ftw: 0x0000 ss: 0x0000000000000000 af: false swd: 0x0000
 frdp: 0x0000000000000000 ac: false zf: false es: 0x0000000000000000 rdi: 0x0000000000000000 sf: false
 r14: 0x0000000000000000 r15: 0x0000000000000000 cf: false r12: 0x0000000000000000 r13: 0x0000000000000000
 r10: 0x0000000000000000 r11: 0x0000000000000000 tf: false dr0: 0x0000000000000000 gs: 0x0000000000000000
 fop: 0x0000 df: false nt: false rcx: 0x0000000000000000 pf: false if: false r9: 0x0000000000000000
 r8: 0x0000000000000000 ds: 0x0000000000000000 cs: 0x0000000000000000 st2: 0x00000000000000000000
 of: false cwd: 0x0000 st3: 0x00000000000000000000 st0: 0x00000000000000000000 rf: false st1: 0x00000000000000000000
 st6: 0x00000000000000000000 rax: 0x0000000000000000 st7: 0x00000000000000000000 vm: false st4: 0x00000000000000000000
 st5: 0x00000000000000000000 rsi: 0x0000000000000000 rbp: 0x0000000000000000

...

Closing issues

Partially closes #2129

...

Copy link
Member

@kazarmy kazarmy left a comment

Choose a reason for hiding this comment

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

[0x000061d0]> e log.file=/dev/null log.level=5
[0x000061d0]> e log.file
/dev/null log.level=5

This is bad and generally Rizin shouldn't react like this.

@wargio
Copy link
Member

wargio commented Apr 21, 2024

This is bad and generally Rizin shouldn't react like this.

i think this is the correct behavior, but also that should have failed (because it is not a valid file).

that said, also aezv rbx = rbx = rbx = PC is a nice improvement but i wonder if this should be part of the grammar or not. @XVilka @ret2libc @thestr4ng3r

@treseco
Copy link
Author

treseco commented Apr 21, 2024

that said, also aezv rbx = rbx = rbx = PC is a nice improvement but i wonder if this should be part of the grammar or not.

This is the same as what ar and dr do, so any changes should be kept consistent across all three.

This is bad and generally Rizin shouldn't react like this.

Since the prior version didn't allow for whitespace in the values it should be possible to check for whitespace after the value is trimmed and either warn or error if there is any.

Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

LGTM. @kazarmy FLAG_LAST was added just to support cases like this and it is widely used automatically in many cases.

librz/core/cmd_descs/cmd_analysis.yaml Outdated Show resolved Hide resolved
Copy link
Member

@kazarmy kazarmy left a comment

Choose a reason for hiding this comment

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

@ret2libc I was complaining more about the possibly bad user experience ... I do understand that both the old and new ways can be implemented (if there is a good reason) and I do not actually have strong feelings either way (meh maybe later) ... though I do have a preference for less keystrokes if possible

@treseco treseco force-pushed the unify-shell-assignment branch from 2ef818b to 6019fe0 Compare April 24, 2024 13:28
@treseco
Copy link
Author

treseco commented Apr 24, 2024

@kazarmy I see your point, I think there is a better solution here. Since changing $ would be the most involved and changing e is less than ideal It's probably better to keep them the same, as they already behave similarly. That way ar, dr, and aezv can be changed to use equals without spaces as well. My only question then is if these commands should also support multiple assignment like e. Doing so would mean they inherit this kind of behavior from e:

[0x100003c0c]> e log.level log.file log.file=/dev/null log.file asm.lines
5

/dev/null
true

Either way all the commands would be using equals with no spaces at that point. Let me know if its preferred to switch to this version and I'll go ahead and make this to a draft.

@kazarmy
Copy link
Member

kazarmy commented Apr 25, 2024

@treseco No HCI experiment has been done regarding this afaik (whether formal or informal) and so like the issue says, you can resolve it either way. Problems found later can be fixed later.

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

Successfully merging this pull request may close these issues.

Unify assignment command code
4 participants