Imperial-MEng/pintos_22
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix stack initialization to correctness of passing argument tests #28

Closed
td1223 wants to merge 6 commits from user-programs-stdout into user-programs
pull from: user-programs-stdout
merge into: :user-programs
Conversation 9 Commits 6 Files Changed 2
+13 -12
td1223 commented 2024-11-07 00:53:05 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Fix stack initialization to pass stack addreses (rather than thread addresses) for the arguments and only pass name a single time

Fix stack initialization to pass stack addreses (rather than thread addresses) for the arguments and only pass name a single time
td1223 commented 2024-11-07 00:53:05 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

assigned to @gk1623

assigned to @gk1623
gk1623 commented 2024-11-07 16:57:18 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Lets extract 15 to a constant (avoiding magic numbers)?

Or perhaps at least sizeof (file_name) to get the array size (since it's a direct array type)?

Lets extract `15` to a constant (avoiding magic numbers)? Or perhaps at least `sizeof (file_name)` to get the array size (since it's a direct array type)?
gk1623 commented 2024-11-07 16:57:19 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Is this required?

Is this required?
gk1623 commented 2024-11-07 16:57:19 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Why are we keeping track of argv at all, when during all the time that we use its value, it is equal to if_.esp?

Why are we keeping track of `argv` at all, when during all the time that we use its value, it is equal to `if_.esp`?
sb3923 commented 2024-11-07 18:18:46 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Agreed, Perhaps: FILE_NAME_LENGTH_MAX or FILE_NAME_MAX?

then yes in strlcpy I would also use sizeof.

I also agree with the comment to move the file naming to process_execute. So the other strlcpy line is to be moved there. Would still use sizeof though.

Agreed, Perhaps: FILE_NAME_LENGTH_MAX or FILE_NAME_MAX? then yes in strlcpy I would also use sizeof. I also agree with the comment to move the file naming to process_execute. So the other strlcpy line is to be moved there. Would still use sizeof though.
sb3923 commented 2024-11-07 18:20:12 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

We must print the exit status as per the spec. But if you mean if it is required here, I agree with you that it is not, and should be instead left to process_exit. See the other merge request.

We must print the exit status as per the spec. But if you mean if it is required here, I agree with you that it is not, and should be instead left to `process_exit`. See the other merge request.
sb3923 commented 2024-11-07 18:22:04 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

When I have a chance, I will also have a look at the process file, but maybe this will be after I am done with syscalls. So without reading the code, overall, try to extract logic into static functions, e.g., setting up the args, or lifting into the other functions when relevant.

When I have a chance, I will also have a look at the process file, but maybe this will be after I am done with syscalls. So without reading the code, overall, try to extract logic into static functions, e.g., setting up the args, or lifting into the other functions when relevant.
gk1623 commented 2024-11-08 04:27:49 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

sorry forgot that's now how sizeof works 😅 . I think I prefer just having the constant everywhere, wby?

(otherwise we need to "assume" that char = byte, or have the rather ugly sizeof (file_name) / sizeof (file_name[0]))

sorry forgot that's now how `sizeof` works :sweat_smile: . I think I prefer just having the constant everywhere, wby? (otherwise we need to "assume" that char = byte, or have the rather ugly `sizeof (file_name) / sizeof (file_name[0])`)
sb3923 commented 2024-11-08 13:31:02 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Yeah that's very ugly. I think constant everywhere is the best approach too.

Yeah that's very ugly. I think constant everywhere is the best approach too.
td1223 (Migrated from gitlab.doc.ic.ac.uk) closed this pull request 2024-11-08 15:14:23 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
cyclane (Gleb Koval)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Imperial-MEng/pintos_22#28