[#4] Implement argument passing #39

Lment · 2017-04-05T16:50:41Z

Merged branch [#38] in local, which fixed unintended argument manipulation.

Using pintos -v -k -T 60 --bochs --fs-disk=2 -p ../../examples/echo -a echo -- -q -f run 'echo xxx' and hex_dump(), current implementation is working well with argc = 2, and since implementation is based on make argument passing when argc = n, I expect this works well with another user programs.

Use hex_dump((uintptr_t)(PHYS_BASE - 200), (void **) (PHYS_BASE - 200), 250, true); in bottom of push_arg_on_stack().

hangpark · 2017-04-06T12:25:18Z

src/userprog/process.c

+
+#define WORD_SIZE sizeof(uintptr_t)
+
+struct arguments


Make this static

hangpark · 2017-04-06T12:42:05Z

src/userprog/process.c

+
+#define WORD_SIZE sizeof(uintptr_t)
+
+struct arguments


Write a comment.

hangpark · 2017-04-06T12:44:10Z

src/userprog/process.c

+static void argument_parser(char *str, struct arguments *args);
+
+static void
+argument_parser (char *string_input, struct arguments *argument)


Move this method below start_process() for consistency.

hangpark · 2017-04-06T12:44:30Z

src/userprog/process.c

+static void argument_parser(char *str, struct arguments *args);
+
+static void
+argument_parser (char *string_input, struct arguments *argument)


Write a comment.

hangpark · 2017-04-06T12:55:34Z

src/userprog/process.c


+  process_name = strtok_r (process_name, " ", &leftover);


Use argument_parse() here. Duplicated.

hangpark · 2017-04-06T13:42:29Z

src/userprog/process.c

+    {
+      arg_length = strlen (args->argv[i]) + 1;
+      dest = top - arg_length;
+      memcpy ((void *) dest, (void *) (args->argv[i]), arg_length);


(void *) is now useless.

hangpark · 2017-04-06T13:46:30Z

src/userprog/process.c

+  for(i = 0; i < alignment ; i++)
+    {
+      memcpy ((top - 1), &zero, 1);
+      top--;


memcpy (--top, &zero, 1);

hangpark · 2017-04-06T13:48:21Z

src/userprog/process.c

+    }
+
+  memcpy ((top - WORD_SIZE), &zero, WORD_SIZE);
+  top -= WORD_SIZE;


Switch above two statements.

hangpark · 2017-04-06T13:48:43Z

src/userprog/process.c

+  for(i = (args->argc -1); i >= 0; i--)
+    {
+      memcpy ((top - WORD_SIZE), &argument_pointers[i], WORD_SIZE);
+      top -= WORD_SIZE;


Switch above two statements.

hangpark · 2017-04-06T13:51:15Z

src/userprog/process.c

+  top -= WORD_SIZE;
+
+  memcpy ((top - WORD_SIZE), &zero, WORD_SIZE);
+  top -= WORD_SIZE;


Switch two statements in each of 3 groups above.

hangpark

Add full commit message.

hangpark

Add little comment in

hangpark · 2017-04-06T13:59:10Z

src/userprog/process.c

@@ -464,3 +513,55 @@ install_page (void *upage, void *kpage, bool writable)
  return (pagedir_get_page (t->pagedir, upage) == NULL
          && pagedir_set_page (t->pagedir, upage, kpage, writable));
 }
+
+static void
+push_arg_on_stack(struct arguments *args, void **esp)


Rename push_args_to_stack and rename all occurrence of argument and arg above to args appropriately.

hangpark · 2017-04-06T13:59:48Z

src/userprog/process.c

+  int i;
+  char *dest;
+
+  for(i = (args->argc - 1); i >= 0; i--)


Write little comment with // to specify which data is going to be pushed.

hangpark · 2017-04-07T07:51:45Z

src/userprog/process.c

-static bool load (const char *cmdline, void (**eip) (void), void **esp);
+static bool load (struct arguments *argument, void (**eip) (void), void **esp);
+
+static void argument_parser(char *str, struct arguments *args);


Move this prototype above start_process.

hangpark · 2017-04-07T07:54:42Z

src/userprog/process.c

@@ -29,6 +41,7 @@ tid_t
 process_execute (const char *file_name) 
 {
  char *fn_copy;
+  struct arguments *args = malloc (sizeof *args);


change right-hand side to (struct arguments *) malloc (sizeof struct arguments).

I referenced here. Also, this doesn't work. Casting is o.k, but in sizeof, this code invokes syntax error before struct.

hangpark · 2017-04-07T07:58:01Z

src/userprog/process.c

@@ -38,34 +51,66 @@ process_execute (const char *file_name)
    return TID_ERROR;
  strlcpy (fn_copy, file_name, PGSIZE);

+  argument_parser (fn_copy, args); 


Would be better to give a short comment above this statement. How do you think?

Added a comment.

hangpark · 2017-04-07T08:00:14Z

src/userprog/process.c

  if (tid == TID_ERROR)
-    palloc_free_page (fn_copy); 
+    {
+      palloc_free_page (fn_copy); 


Should be freed after below two frees.

hangpark · 2017-04-07T08:03:43Z

src/userprog/process.c

  return tid;
 }

+/* Starts string tokenizer, which cuts given input string into tokens 


Don't need to explain which library function did you use. Writing about what does this method do is better.

ex) Parse a given command line in string to strings of each argument by replacing every spaces to the null character. And then store them in the arguments structure.

hangpark · 2017-04-07T08:41:26Z

src/userprog/process.c

+static void
+push_args_on_stack(struct arguments *args, void **esp)
+{
+  void **arg_ptrs = calloc (args->argc, sizeof(uintptr_t));


Just use

void **arg_ptrs = (void **) calloc (args->argc, sizeof void *);

Invokes syntax error before void, when following this problem.

hangpark · 2017-04-07T08:44:44Z

src/userprog/process.c

+
+  alignment = ((uint32_t) curr) % WORD_SIZE;
+
+  /* Aligning, and push bytes required for aligning. */


Should move above alignment initialization.

Use push, save. Not pushing, saving.

hangpark · 2017-04-07T08:45:23Z

src/userprog/process.c

+  int i;
+  void *dest;
+
+  /* Pushing arguments on stack, saving their address in arg_ptrs. */


Use push, save. Not pushing, saving.

hangpark · 2017-04-07T08:46:08Z

src/userprog/process.c

+  for(i = 0; i < alignment ; i++)
+    memcpy (--curr, &zero, 1);
+
+  curr -= WORD_SIZE;


What is this for? missing comment.

hangpark · 2017-04-07T08:46:25Z

src/userprog/process.c

+  curr -= WORD_SIZE;
+  memcpy (curr, &zero, WORD_SIZE);
+
+  for(i = (args->argc -1); i >= 0; i--)


What is this for? missing comment.

hangpark

Why indent is tab?

hangpark · 2017-04-07T11:15:36Z

src/userprog/process.c

+
+  /* esp pointing bottom of the stack */
+  *esp = (void *) curr;  
+  hex_dump((uintptr_t)(PHYS_BASE - 200), (void **) (PHYS_BASE - 200), 250, true);


hangpark · 2017-04-07T11:25:07Z

src/userprog/process.c

  return tid;
 }

+/* Parse arguments from given string, which removes whole adjacent spaces and replace it  
+   with NULL. And then stores it in given argument struct by pointer. */


Not NULL. '\0' or null character.

hangpark · 2017-04-07T11:25:56Z

src/userprog/process.c

+
+  for (curr = strtok_r (str_input, delim, &leftover);
+       curr != NULL; curr = strtok_r (NULL, delim, &leftover))
+    args->argv[args->argc ++] = curr;


Remove space

hangpark · 2017-04-07T11:26:59Z

src/userprog/process.c

+  delim = " ";
+
+  for (curr = strtok_r (str_input, delim, &leftover);
+       curr != NULL; curr = strtok_r (NULL, delim, &leftover))


Does first line > 80? If not, move curr != NULL above.

hangpark · 2017-04-07T11:27:27Z

src/userprog/process.c

+  args->argc = 0;
+  args->argv = (char **) calloc ((strlen (str_input) + 1), sizeof (char));
+
+  delim = " ";


Just use in strtok_r

hangpark · 2017-04-07T11:28:07Z

src/userprog/process.c

  struct intr_frame if_;
  bool success;

+


hangpark · 2017-04-07T11:28:58Z

src/userprog/process.c

+/* Push arguments on newly initialized stack, following calling conventions.
+  This invention does not use esp dynamically, just making it point bottom of 
+  stack frame after all push is done.*/
+


hangpark · 2017-04-07T11:29:45Z

src/userprog/process.c

+push_args_on_stack(struct arguments *args, void **esp)
+{
+  void **arg_ptrs = (void **) calloc (args->argc, sizeof (void *));
+  void *curr = (char *) PHYS_BASE;


Use esp, not PHYS_BASE.

hangpark · 2017-04-07T11:30:46Z

src/userprog/process.c

+  void *dest;
+
+  /* Push arguments on stack, save their address in arg_ptrs. */
+  for(i = (args->argc - 1); i >= 0; i--)


Space after for, remove parenthesis wrapping args->argc - 1.

hangpark · 2017-04-07T11:31:08Z

src/userprog/process.c

+    {
+      arg_len = strlen (args->argv[i]) + 1;
+      dest = curr - arg_len;
+      memcpy (dest, (args->argv[i]), arg_len);


Remove parenthesis

hangpark

You're using curr -= WORD_SIZE and --curr both. Use just one. I prefer second one. And then you might not need WORD_SIZE.

hangpark · 2017-04-07T12:03:06Z

src/userprog/process.c

+    int argc;
+    char **argv;
+  };
+


No blank line.

hangpark · 2017-04-07T12:03:52Z

src/userprog/process.c

-static bool load (const char *cmdline, void (**eip) (void), void **esp);
+static bool load (struct arguments *args, void (**eip) (void), void **esp);
+
+static void argument_parser(char *str, struct arguments *args);


I commented that this method should be above start_process but this has not been fixed yet.

The method is now part of process_execute(), and not called in start_process(). And, in load(), all of its helper function prototypes are defined just above its declaration. So I placed this prototype here, just above process_execute().

I'll follow you. then, plz remove gap between load() and argument_parser().

hangpark · 2017-04-07T12:04:24Z

src/userprog/process.c

@@ -29,6 +43,7 @@ tid_t
 process_execute (const char *file_name) 
 {
  char *fn_copy;
+  struct arguments *args =  (struct arguments *) malloc (sizeof *args);


I saw your reply now. Please tag me with # symbol for outdated comment 😃

But still, then does sizeof (struct arguments) invoke an error? If not, I'd like to recommend you to use this instead of getting size from variable args by sizeof *args. What do you think about it?

I also tried it, wondering why sizeof cannot calculate a size of structure pointers, but it still invoked an error, with same message. Following this now, since I can't figure out how I could get size of structure pointer without this syntax.

Is your error appearing during building?

struct arguments *args = (struct arguments *) malloc (sizeof (struct arguments));

This works very well in my env. Please let me know your error.

Seems to be problem with this. Since this problem was solved, the above solution now works. Changing it as suggested.

👍
Be careful limit length of line (80)!!!!

hangpark · 2017-04-07T12:06:58Z

src/userprog/process.c

@@ -38,19 +53,43 @@ process_execute (const char *file_name)
    return TID_ERROR;
  strlcpy (fn_copy, file_name, PGSIZE);

+  /* Parse argument, save it in structure. */


How about Parse arguments into the structure?

hangpark · 2017-04-07T12:08:14Z

src/userprog/process.c

+      free(args);
+      palloc_free_page (fn_copy); 
+    }
+


Remove this line.

hangpark · 2017-04-07T12:19:43Z

src/userprog/process.c

+  char *curr;
+
+  args->argc = 0;
+  args->argv = (char **) calloc ((strlen (str_input) + 1), sizeof (char));


Why didn't you fix this code as I requested?

char * is changed to char. Previously it was char *. It is a minimum memory for argv, I suppose.

I mean parenthesis.

hangpark · 2017-04-07T12:20:56Z

src/userprog/process.c

+  args->argv = (char **) calloc ((strlen (str_input) + 1), sizeof (char));
+
+  for (curr = strtok_r (str_input, " ", &leftover);
+       curr != NULL; curr = strtok_r (NULL, " ", &leftover))


Why didn't you fix this code as I requested? Please always write the reply why you disagree my request.

This line is about 113 or more characters which is longer than 80, so it should be divided.

I mean CHANGE

for (curr = strtok_r (str_input, " ", &leftover); curr != NULL; curr = strtok_r (NULL, " ", &leftover))

TO

for (curr = strtok_r (str_input, " ", &leftover); curr != NULL; curr = strtok_r (NULL, " ", &leftover))

NOT

for (curr = strtok_r (str_input, " ", &leftover); curr != NULL; curr = strtok_r (NULL, " ", &leftover))

hangpark · 2017-04-07T12:22:16Z

src/userprog/process.c

+push_args_on_stack(struct arguments *args, void **esp)
+{
+  void **arg_ptrs = (void **) calloc (args->argc, sizeof (void *));
+  void *curr = (char *) *esp;


Why (char *)?

hangpark · 2017-04-07T12:23:30Z

src/userprog/process.c

+  alignment = ((uint32_t) curr) % WORD_SIZE;
+  for(i = 0; i < alignment ; i++)
+    { 
+      curr -= 1;


Oh, that was my mistake. Sorry. Please combine these two into memcpy (--curr, ~~~~~).

hangpark · 2017-04-07T13:09:17Z

src/userprog/process.c

@@ -17,9 +17,23 @@
 #include "threads/palloc.h"
 #include "threads/thread.h"
 #include "threads/vaddr.h"
+#include "threads/malloc.h"
+
+#define WORD_SIZE sizeof(uintptr_t)


sizeof uintptr_t

sizeof uintptr_t invokes syntax error when referencing WORD_SIZE. Similarly, sizeof char too. It seems it needs parenthesis to count something's size.

Then, why no spacing? 😠

hangpark · 2017-04-07T13:10:42Z

src/userprog/process.c

+
+#define WORD_SIZE sizeof(uintptr_t)
+
+/* Structure for argument, which saves arguments in argv,


It does not count the number of args. Just saves it.

Structure for arguments. would be enough I think. And add right comment (//) for each member like in threads/thread.h

hangpark

You should update your local branch to the origin.

hangpark · 2017-04-07T18:49:20Z

src/userprog/process.c

+struct arguments
+  { 
+    int argc;                /* Argument counter which saves calculated number of arguments. */ 
+    char **argv;             /* Argument vector, which saves arguments in array of strings. */


Does these two lines fit in 80 letters?

hangpark · 2017-04-07T18:49:58Z

src/userprog/process.c

+#define WORD_SIZE sizeof (uintptr_t)
+
+/* Structure for arguments, which saves arguments in argv,
+   and saves the number of arguments. */


Does not need which saves ~~~ of arguments.

hangpark · 2017-04-07T18:51:16Z

src/userprog/process.c

+   and saves the number of arguments. */
+struct arguments
+  { 
+    int argc;                /* Argument counter which saves calculated number of arguments. */ 


Number of arguments. is enough.

hangpark · 2017-04-07T18:51:41Z

src/userprog/process.c

+struct arguments
+  { 
+    int argc;                /* Argument counter which saves calculated number of arguments. */ 
+    char **argv;             /* Argument vector, which saves arguments in array of strings. */


Array of arguments. is enough.

hangpark · 2017-04-07T18:55:40Z

src/userprog/process.c

@@ -29,6 +41,7 @@ tid_t
 process_execute (const char *file_name) 
 {
  char *fn_copy;
+  struct arguments *args =  (struct arguments *) malloc (sizeof (struct arguments));


Does this line fit in 80 letters?

@hangpark this line has 84 characters, So it should be divided.
In my opinion :

struct arguments *args = (struct arguments *) malloc (sizeof (struct arguments)); struct arguments *args = (struct arguments *) malloc (sizeof (struct arguments)); struct arguments *args = (struct arguments *) malloc (sizeof (struct arguments));

could be used to follow convention. What would you suggest? I think third one is most appropriate.

Just separate declaration and initialization.

hangpark · 2017-04-07T19:20:46Z

src/userprog/process.c

@@ -38,19 +51,42 @@ process_execute (const char *file_name)
    return TID_ERROR;
  strlcpy (fn_copy, file_name, PGSIZE);

+  /* Parse argument into the structure. */
+  argument_parser (fn_copy, args); 


Move initialization to here, between comment and caller.

hangpark

Almost done....!! 😆

hangpark · 2017-04-08T06:45:38Z

src/userprog/process.c

+      curr -= WORD_SIZE;
+      memcpy (curr, &arg_ptrs[i], WORD_SIZE);
+    }
+


Removing this line would be better 😃

hangpark · 2017-04-08T06:46:08Z

src/userprog/process.c

+  curr -= WORD_SIZE;
+  memcpy (curr, &zero, WORD_SIZE);
+
+  /* esp pointing bottom of the stack */


Add comma at the last and change esp to ESP.

hangpark · 2017-04-08T06:46:49Z

src/userprog/process.c

+
+/* Push arguments on newly initialized stack, following calling conventions.
+  This invention does not use esp dynamically, just making it point bottom of 
+  stack frame after all push is done.*/


Second and third line are not aligned vertically with the first line.

hangpark · 2017-04-08T09:27:48Z

src/userprog/process.c

+  curr -= WORD_SIZE;
+  memcpy (curr, &zero, WORD_SIZE);
+
+  /* Esp pointing bottom of the stack */


Why din't you fix all my requests?

hangpark · 2017-04-08T10:27:00Z

src/userprog/process.c

+  curr -= WORD_SIZE;
+  memcpy (curr, &zero, WORD_SIZE);
+
+  /* esp pointing bottom of the stack. */


Your code were
esp pointing bottom of the stack no comma
and I want
ESP pointing bottom of the stack .

But you changed yours to
Esp pointing bottom of the stack no comma

and I asked once again, you gave
esp pointing bottom of the stack .
without any reason.

What's the reason? If there is no reason, what's the problem with you?

My fault. I misunderstood the comment.

hangpark · 2017-04-08T11:30:19Z

src/userprog/process.c

+  curr -= WORD_SIZE;
+  memcpy (curr, &zero, WORD_SIZE);
+
+  /* Esp pointing bottom of the stack. */


Your code were
esp pointing bottom of the stack no comma
and I want
ESP pointing bottom of the stack .

But you changed yours to
Esp pointing bottom of the stack no comma

and I asked once again, you gave
esp pointing bottom of the stack .

Finally, you still don't accept my request.
Esp pointing bottom of the stack .

Compare my suggestion and yours:
ESP pointing bottom of the stack .
Esp pointing bottom of the stack .

Is this too hard to understand?

Now an execution of process supports multiple arguments, by pushing arguments onto stack that has been previously allocated, but not used. esp now points bottom of the stack frame, which is extended from PHYS_BASE to accommodate arguments and pointers to that arguments. This implementation is now following calling convention, which is defined in pintos manual.

Lment added the feature label Apr 5, 2017

Lment added this to the Project #2 milestone Apr 5, 2017

Lment requested a review from hangpark April 5, 2017 16:50

Lment force-pushed the iss/4 branch 3 times, most recently from 7411c0c to 98648fe Compare April 6, 2017 12:23

hangpark requested changes Apr 6, 2017

View reviewed changes

Lment force-pushed the iss/4 branch from 98648fe to 910ba12 Compare April 7, 2017 04:35

hangpark assigned Lment Apr 7, 2017

Lment force-pushed the iss/4 branch 3 times, most recently from 6823c80 to 346cfa7 Compare April 7, 2017 08:13