Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chapters/{memory-layout,stack}: Add tasks and reading material for C - Assembly interaction #24

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

Conversation

savacezarmarian14
Copy link

@savacezarmarian14 savacezarmarian14 commented May 20, 2024

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

@github-actions github-actions bot added area/tasks Update to tasks topic/memory-layout Related to the "Memory Layout" chapter kind/new New content / item labels May 21, 2024
@teodutu teodutu requested a review from razvang0307 May 22, 2024 08:05
@teodutu teodutu added the needs-rendering The PR makes changes to the website that need to be rendered label May 22, 2024
Copy link

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.

  • Remove all utils/ folders because printf32.asm is no longer used and change the makefiles that include those files to be standalone.
  • Add the following line at the end of each task statement: If you're having difficulties solving this exercise, go through [this](link to a relevant reading/ section) reading material

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.

I submitted my previous review too early. Below is the continuation:

  • For all tasks mention the path where students can find the code (drills/tasks/task-name/support/[file_name]).
  • Use Title Case in all Markdown headings: https://en.wikipedia.org/wiki/Title_case.
  • Remove exercise numbers from max-calls.
  • Fix the linter errors [1]. Some of those coming from checkpatch are false positives, so you can ignore them.
  • Change your commit message to chapters/{memory-layout,stack}: Add tasks and reading material for C - Assembly interaction and add a description of your changes.

[1] https://github.com/cs-pub-ro/hardware-software-interface/pull/24/checks

config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
@savacezarmarian14 savacezarmarian14 force-pushed the conversion-lab-10-interaction-c-assembly branch from e987016 to 3da3c28 Compare July 21, 2024 23:23
@savacezarmarian14 savacezarmarian14 changed the title Created lab10 in new repo chapters/{memory-layout,stack}: Add tasks and reading material for C - Assembly interaction Jul 21, 2024
@teodutu teodutu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Jul 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.

I left a few more suggestions. You haven't addressed all my previous comments yet. I pointed out which of them need addressing. I also made some additional suggestions.

@savacezarmarian14 savacezarmarian14 force-pushed the conversion-lab-10-interaction-c-assembly branch 2 times, most recently from 2134941 to 0aaf4a5 Compare July 23, 2024 09:06
@teodutu teodutu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Jul 24, 2024
Copy link

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.

Pay attention to my older comments that you haven't addressed yet. Click "Resolve" when you apply one of them so you can keep track of them better. I also found a few other incongruencies, mostly related to Makefiles and .gitignores. The text and exercises seem good though.

@@ -0,0 +1,26 @@
# Compilator și flag-uri
Copy link

Choose a reason for hiding this comment

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

Bump.

@@ -0,0 +1 @@
/mainmax
Copy link

Choose a reason for hiding this comment

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

Bump. Also add this file to ../solution.

@@ -0,0 +1 @@
/main
Copy link

Choose a reason for hiding this comment

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

Bump. And copy this file to ../solution/

@teodutu
Copy link

teodutu commented Jul 24, 2024

And rebase the main branch. Otherwise we can't merge this once the fixes are made.

@savacezarmarian14 savacezarmarian14 force-pushed the conversion-lab-10-interaction-c-assembly branch 5 times, most recently from fba0644 to ae10021 Compare July 30, 2024 18:40
Copy link

@NickZaharia308 NickZaharia308 left a comment

Choose a reason for hiding this comment

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

Overall good 👍

@teodutu teodutu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Aug 5, 2024
Copy link

github-actions bot commented Aug 5, 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.

I made a few more corrections. Besides those, fix the CI/CD errors:

  • checkpatch fails for some Makefiles where rm -f main main.o can be replaced with rm -f main *.o [1]
  • super-linter complains about block quotes where you can simply add > at the beginning of empty lines [2]
  • You can skip spellcheck after addressing all comments because mainmax is a false positive [3]

[1] https://github.com/cs-pub-ro/hardware-software-interface/actions/runs/10167959782/job/28121457526?pr=24
[2] https://github.com/cs-pub-ro/hardware-software-interface/actions/runs/10167959782/job/28121456962?pr=24
[3] https://github.com/cs-pub-ro/hardware-software-interface/actions/runs/10167959782/job/28121457272?pr=24

… to point to the path of the exercices. Also modified config.yaml and removed guides from this lab.
@savacezarmarian14 savacezarmarian14 force-pushed the conversion-lab-10-interaction-c-assembly branch from ae10021 to bb5ca28 Compare August 12, 2024 10:45
@teodutu teodutu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Aug 15, 2024
Copy link

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.

I made suggestions for what needs to change for the linters to pass.
In addition, take care of the following:

  • To fix spellcheck, make a PR to add extern to this wordlist [1] and mainmax to this one [2].
  • Change your commit message to:
chapters/{memory-layout,stack}: Add tasks and reading material for C - Assembly interaction

Then add a description of the changes below and sign it with git commit -as

[1] https://github.com/open-education-hub/actions/blob/main/spellcheck/config/technical.txt
[2] https://github.com/open-education-hub/actions/blob/main/spellcheck/config/acronyms.txt

@@ -0,0 +1 @@
mainmax
Copy link

Choose a reason for hiding this comment

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

Suggested change
mainmax
mainmax

Add a newline at the end of this file

@@ -0,0 +1 @@
mainmax
Copy link

Choose a reason for hiding this comment

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

Suggested change
mainmax
mainmax

Add a newline at the end of this file

The printed message is not as expected because the assembly code is missing an instruction.

Use GDB to inspect the address at the top of the stack before executing the `ret` statement in the `print_hello()` function.
What does it point to?
Copy link

Choose a reason for hiding this comment

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

Suggested change
What does it point to?
What does it point to?

@@ -0,0 +1 @@
main
Copy link

Choose a reason for hiding this comment

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

Suggested change
main
main

Add a newline at the end of this file

@@ -0,0 +1,18 @@
// SPDX-License-Identifier: BSD-3-Clause

extern int puts(const char *);
Copy link

Choose a reason for hiding this comment

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

Suggested change
extern int puts(const char *);
extern int puts(const char *str);

According to the checkpatch error.

> **IMPORTANT:**
> The return value of a function is placed in the `eax` register.

# Maximum Computation Extension in Assembly with Call from C
Copy link

Choose a reason for hiding this comment

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

Suggested change
# Maximum Computation Extension in Assembly with Call from C
## Maximum Computation Extension in Assembly with Call from C

> **TIP:**
> In order to restore the stack to its state at the start of the current function, the `leave` statement relies on the function's pointer frame having been set.

If you're having difficulties solving this exercise, go through [this relevant section](../../../reading/README.md#passing-parameters-from-c-to-the-assembly-procedure) reading material.
Copy link

Choose a reason for hiding this comment

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

Suggested change
If you're having difficulties solving this exercise, go through [this relevant section](../../../reading/README.md#passing-parameters-from-c-to-the-assembly-procedure) reading material.
If you're having difficulties solving this exercise, go through [this relevant section](../../../reading/README.md#passing-parameters-from-c-to-the-assembly-procedure) reading material.

When C executes the call to `sum()`, it first pushes arguments on the stack in reverse order, then actually calls the procedure.
Thus, upon entering the procedure body, the stack will be intact.

Since the variables `a` and `b` are declared as `int` values, they will each use one word on the stack.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Since the variables `a` and `b` are declared as `int` values, they will each use one word on the stack.
Since the variables `a` and `b` are declared as `int` values, they will each use one word on the stack.

> pos: dd 0
> ```
>
> This variable you will pass (by address) to the `get_max` call and by value to the `printf()` call for display.
Copy link

Choose a reason for hiding this comment

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

Suggested change
> This variable you will pass (by address) to the `get_max` call and by value to the `printf()` call for display.
> This variable you will pass (by address) to the `get_max()` call and by value to the `printf()` call for display.


Use GDB to inspect the address at the top of the stack before executing the `ret` statement in the `print_hello()` function.
What does it point to?
Track the values of the EBP and ESP registers during the execution of this 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
Track the values of the EBP and ESP registers during the execution of this function.
Track the values of the `ebp` and `esp` registers during the execution of this function.

@teodutu
Copy link

teodutu commented Aug 27, 2024

@savacezarmarian14 address my last review and then we can merge this

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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/memory-layout Related to the "Memory Layout" chapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants