Fix stack initialization to correctness of passing argument tests #28

Closed
td1223 wants to merge 6 commits from user-programs-stdout into user-programs
td1223 commented 2024-11-07 00:53:05 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

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)

assigned to @gk1623

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

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)

Is this required?

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

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)

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)

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)

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)

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)

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

Sign in to join this conversation.
No Reviewers
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Imperial-MEng/pintos_22#28
No description provided.