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

stack/functions: Add functions lab. #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

razvang0307
Copy link

Added functions lab based off old lab material.
Ignored throwback exercise 0.

Prerequisite Checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Updated relevant documentation (if needed).

Description of changes

Added the lab materials for function calls in assembly.

@razvang0307 razvang0307 added the needs-rendering The PR makes changes to the website that need to be rendered label Apr 21, 2024
Copy link

Added functions lab based off old lab material.
Ignored throwback exercise 0.
Squashed commits into one.

Signed-off-by: Razvan Miclius <[email protected]>
@github-actions github-actions bot added area/guides Update to guides area/tasks Update to tasks topic/stack Related to the "Stack" chapter kind/new New content / item labels Apr 22, 2024
Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

Things are looking nice so far. I highlighted what needs to be done in my inline comments and besides those, you should also do the following globally:

  • Add a .gitignore file to all support/ and solution/ folders. This file should only exclude the executable file and not the object file (.o) because object files are already excluded by the .gitignore in the root of the repo.
  • Take a look at the linter errors and fix them [1]. To fix the Spellcheck errors you'll also have to add the false positives (correct words flagged as incorrect because they're missing from the action's wordlist) to the wordlists here. Create a PR in which you add the new words to the corresponding files here [2]. After changing a file, run sort -u -f <file_name.txt> <file_name.txt> to remove duplicates. Remember to add the conjugations and declinations of each word as well as the word itself.
  • Squash your commits into a single one as described here [3]. For further modifications, use git commit --amend --no-edit to add them to the existing commit.

[1] https://github.com/cs-pub-ro/hardware-software-interface/pull/21/checks
[2] https://github.com/open-education-hub/actions/tree/main/spellcheck/config
[3] https://www.freecodecamp.org/news/git-squash-commits/

Copy link

Choose a reason for hiding this comment

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

Change the name of the folder to print-rev-string (using dashes -, not underscores _). Do the same for all folder names.

Comment on lines +14 to +22
mov eax, dword [ebp+8]
mov ecx, dword [ebp+12]
add eax, ecx
dec eax
mov edx, dword [ebp+16]

copy_one_byte:
mov bl, byte [eax]
mov byte [edx], bl
Copy link

Choose a reason for hiding this comment

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

Suggested change
mov eax, dword [ebp+8]
mov ecx, dword [ebp+12]
add eax, ecx
dec eax
mov edx, dword [ebp+16]
copy_one_byte:
mov bl, byte [eax]
mov byte [edx], bl
mov eax, [ebp + 8]
mov ecx, [ebp + 12]
add eax, ecx
dec eax
mov edx, [ebp + 16]
copy_one_byte:
mov bl, [eax]
mov [edx], bl

Using dword pr byte here is pointless because they can be inferred due to the other operand being a register.

@@ -0,0 +1,59 @@
# Displaying the Reversed String

In the file `print_rev_string.asm`, add the `reverse_string` function so that you have a listing similar to the one below:
Copy link

Choose a reason for hiding this comment

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

Suggested change
In the file `print_rev_string.asm`, add the `reverse_string` function so that you have a listing similar to the one below:
In the file `print_rev_string.asm`, add the `reverse_string()` function so that you have a listing similar to the one below:

Use () at the end of function names to distinguish them better from simple labels.

[...]
```

> **IMPORTANT:** When copying the `reverse_string` function into your program, remember that the function starts at the `reverse_string` label and ends at the `main` label. The `copy_one_byte` label is part of the `reverse_string` function.
Copy link

Choose a reason for hiding this comment

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

Suggested change
> **IMPORTANT:** When copying the `reverse_string` function into your program, remember that the function starts at the `reverse_string` label and ends at the `main` label. The `copy_one_byte` label is part of the `reverse_string` function.
> **IMPORTANT:** When copying the `reverse_string()` function into your program, remember that the function starts at the `reverse_string` label and ends at the `main` label.
> The `copy_one_byte` label is part of the `reverse_string` function.

Write one sentence per line. Everywhere. Also, notice the distinction I made above between the reverse_string() (with ()) function and the reverse_string label without (()).

Comment on lines +16 to +51
mov eax, dword [ebp+8]
mov ecx, dword [ebp+12]
add eax, ecx
dec eax
mov edx, dword [ebp+16]

copy_one_byte:
mov bl, byte [eax]
mov byte [edx], bl
dec eax
inc edx
loopnz copy_one_byte

inc edx
mov byte [edx], 0

leave
ret

main:
push ebp
mov ebp, esp

mov eax, mystring
xor ecx, ecx
test_one_byte:
mov bl, byte [eax]
test bl, bl
jz out
inc eax
inc ecx
jmp test_one_byte

out:
; save ecx's value since it can be changed by printf
push ecx
Copy link

Choose a reason for hiding this comment

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

Suggested change
mov eax, dword [ebp+8]
mov ecx, dword [ebp+12]
add eax, ecx
dec eax
mov edx, dword [ebp+16]
copy_one_byte:
mov bl, byte [eax]
mov byte [edx], bl
dec eax
inc edx
loopnz copy_one_byte
inc edx
mov byte [edx], 0
leave
ret
main:
push ebp
mov ebp, esp
mov eax, mystring
xor ecx, ecx
test_one_byte:
mov bl, byte [eax]
test bl, bl
jz out
inc eax
inc ecx
jmp test_one_byte
out:
; save ecx's value since it can be changed by printf
push ecx
mov eax, [ebp + 8]
mov ecx, [ebp + 12]
add eax, ecx
dec eax
mov edx, [ebp + 16]
copy_one_byte:
mov bl, [eax]
mov [edx], bl
dec eax
inc edx
loopnz copy_one_byte
inc edx
mov [edx], 0
leave
ret
main:
push ebp
mov ebp, esp
mov eax, mystring
xor ecx, ecx
test_one_byte:
mov bl, [eax]
test bl, bl
jz out
inc eax
inc ecx
jmp test_one_byte
out:
; Save ecx's value since it can be changed by printf.
push ecx

>
> Stop when you reach the value `0` (`NULL` byte). For checking, you can use `test` as seen in the implementation of determining the length of a string in the `print-string-length.asm` file.

## Bonus: toupper Only for Lowercase Letters
Copy link

Choose a reason for hiding this comment

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

Suggested change
## Bonus: toupper Only for Lowercase Letters
## `toupper()` Only for Lowercase Letters

@@ -0,0 +1,17 @@
# Implementing the toupper Function
Copy link

Choose a reason for hiding this comment

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

Suggested change
# Implementing the toupper Function
# Implementing the `toupper()` Function

Comment on lines +15 to +21
mov eax, dword [ebp+8]
check_one_byte:
mov bl, byte [eax]
test bl, bl
je out
sub bl, 0x20
mov byte [eax], bl
Copy link

Choose a reason for hiding this comment

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

Suggested change
mov eax, dword [ebp+8]
check_one_byte:
mov bl, byte [eax]
test bl, bl
je out
sub bl, 0x20
mov byte [eax], bl
mov eax, [ebp + 8]
check_one_byte:
mov bl, [eax]
test bl, bl
je out
sub bl, 0x20
mov [eax], bl

Copy link

Choose a reason for hiding this comment

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

Increase the font of all text to 20 instead of 12.

Comment on lines +155 to +157
mov eax, dword [ebp+8] ; first argument in eax
mov ebx, dword [ebp+12] ; second argument in ebx
mov ecx, dword [ebp+16] ; third argument in ecx
Copy link

Choose a reason for hiding this comment

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

Suggested change
mov eax, dword [ebp+8] ; first argument in eax
mov ebx, dword [ebp+12] ; second argument in ebx
mov ecx, dword [ebp+16] ; third argument in ecx
mov eax, [ebp + 8] ; first argument in eax
mov ebx, [ebp + 12] ; second argument in ebx
mov ecx, [ebp + 16] ; third argument in ecx

Choose a reason for hiding this comment

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

I slightly disagree. While not necessary, I feel it does decently illustrate the point that we are extracting the arguments from memory.

Copy link

@george-cosma george-cosma left a comment

Choose a reason for hiding this comment

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

Yhea, most of it is fine. Good work! I left some comments though that I'd like your input on. Feel free to disagree.

- It's slow because it involves memory access.
- More complicated in terms of parameter access.

> **NOTE:** For **32-bit** architectures, the stack passing method is used, while for **64-bit** architectures, the register passing method is used. We will use the convention for 32-bit architecture.

Choose a reason for hiding this comment

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

AFAIK in 64-bit you pass a handful of arguments by registers, but after you run out, registers are put on the stack. Do double-check me though.

These modifications take place in the callee. Therefore, it is the responsibility of the callee to restore the stack to its old value. Hence, it is customary to have an epilogue that restores the stack to its initial state; this epilogue is:

```Assembly
leave

Choose a reason for hiding this comment

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

I think mentioning what leave actually does (and mentioning the enter commands too) could be helpful to understand what is happening both in "lower-level" assembly, and to highlight the existence of more complexx commands.

https://stackoverflow.com/a/29790275

Comment on lines +155 to +157
mov eax, dword [ebp+8] ; first argument in eax
mov ebx, dword [ebp+12] ; second argument in ebx
mov ecx, dword [ebp+16] ; third argument in ecx

Choose a reason for hiding this comment

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

I slightly disagree. While not necessary, I feel it does decently illustrate the point that we are extracting the arguments from memory.


*Additionally, in some cases, a memory address can be returned to the stack/heap (eg. malloc), or other memory areas, which refer to the desired object after the function call.*

5. A function uses the same hardware registers; therefore, when exiting the function, the values of the registers are no longer the same. To avoid this situation, some/all registers can be saved on the stack.

Choose a reason for hiding this comment

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

I would encourage to mention pusha and popa (and their downsides -- fucks up the eax register).



> **NOTE:** Since assembly languages offer more opportunities, there is a need for calling conventions in x86. The difference between them may consist of the parameter order, how the parameters are passed to the function, which registers need to be preserved by the callee or whether the caller or callee handles stack preparation. More details can be found [here](https://en.wikipedia.org/wiki/X86_calling_conventions) or [here](https://levelup.gitconnected.com/x86-calling-conventions-a34812afe097) if Wikipedia is too mainstream for you.
> For us, the registers `eax`, `ecx`, `edx` are considered **clobbered** (or volatile), and the callee can do whatever it wants to them. On the other hand, the callee has to ensure that `ebx` exits the function with the same value it has entered with.

Choose a reason for hiding this comment

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

I suggest moving this information (what registers we can ovewrite and not) to main-body contents, maybe beneath 5.. It's important info. Though this is more of an aesthetics change - so I don't feel strongly about it.


section .text

extern puts

Choose a reason for hiding this comment

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

Suggested change
extern puts
extern puts ; we need to mark "puts()" as an external function, not one found in this file

I'd add even more details than what I sugested. Make sure that the students understan what extern is. Perhaps even add a section about it in the main content?

@george-cosma
Copy link

george-cosma commented Apr 24, 2024

Oh, and one more comment: Do we need to put the content under Lab9/Functions/{Reading/Guides/...}. Shouldn't it be just Lab9/{Reading/Guides/...}, skip on the unnecessary sub-heading? This is a question to everyone - how do we want to handle this case?

@teodutu
Copy link

teodutu commented Apr 24, 2024

Oh, and one more comment: Do we need to put the content under Lab9/Functions/{Reading/Guides/...}. Shouldn't it be just Lab9/{Reading/Guides/...}, skip on the unnecessary sub-heading? This is a question to everyone - how do we want to handle this case?

Let's keep them all like this for now so all labs (with or without multiple subchapters) are consistent. It's easy to change this in the future so this isn't that big of an issue.

@@ -0,0 +1,71 @@
section .data

Choose a reason for hiding this comment

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

Maybe add some explanation comments if this is the solution. I think students should read the comments rather than copy the code


push mystring
call puts
add esp, 4

Choose a reason for hiding this comment

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

Students will not understand why this add esp, 4 is needed when the call returns

; TODO: call puts on string

leave
ret

Choose a reason for hiding this comment

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

I strongly recommand a new line at the end of file. See [1]
[1] https://gist.github.com/OleksiyRudenko/d51388345ea55767b7672307fe35adf3

Copy link

@savacezarmarian14 savacezarmarian14 left a comment

Choose a reason for hiding this comment

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

Please check my comments. Also keep in mind that i didn't comment at all files that should have changes. IT LOOKS GOOD FROM MY POINT OF VIEW.

@valudimi valudimi added topic/memory-security Related to the "Memory Security" chapter and removed topic/memory-security Related to the "Memory Security" chapter labels May 14, 2024
@teodutu
Copy link

teodutu commented May 22, 2024

ping @razvang0307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/guides Update to guides area/tasks Update to tasks kind/new New content / item needs-rendering The PR makes changes to the website that need to be rendered topic/stack Related to the "Stack" chapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants