Support global variable initialization #13

eecheng87 · 2020-10-14T13:48:06Z

In latest version, there already have .data section which is for storing initialized global variable. As a result, I don't add .bss section which is for uninitialized variable in this pull request.

jserv

Append "Close #12" in git commit message(s). Therefore, you can link a pull request to an issue to show that a fix is in progress and to automatically close the issue when the pull request is merged.
See https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

jserv · 2020-10-14T14:04:02Z

You can remove or change the statement "2. Global variable initialization is not supported." in README.md. You added new TODO as well.

jserv · 2020-10-14T14:06:45Z

tests/global.c

@@ -0,0 +1,13 @@
+int a = 44;


How about dropping tests/global.c and adding the original test in file tests/driver.sh?
I am planning to delete some trivial files in directory tests such as def.c, fib.c, loop.c, and pointer.c, which are already covered in test driver.

However, it still makes sense to generate an executable from tests/hello.c.

Done! Move global initialization into tests/driver.sh

jserv · 2020-10-14T14:12:36Z

You can prepare another git commit which applies this change to simplify the handing in file src/globals.c. The reason why function global_init exists is because the global variable initializations were not supported.

jserv · 2020-10-14T14:44:32Z

src/cfront.c

+        char buffer[10];
+        /* global initialization must be constant */
+        /* TODO: support rvalue as expression e.g. int a = 3 + 2; */
+        if (lex_peek(T_numeric, buffer))


Can you reuse the code in function read_body_assignment? That is, split some logic/operations into a helper function and then invoke the helper for reading the expressions from assignments.

Do I need to do that anyway? The reason I don't like to do that is:

The only duplicate code in read_body_assignment is find_global_var

Future update about global initialization will make more different with read_body_assignment. If I merge now, I guess read_body_assignment will become harder to read

Merge control flow into read_body_assignment for one line re-use makes read_body_assignment more complicated. I don't think one function(find_global_var) reuse worth it.

I agree. Let's stick to the reasonably effective changes.

eecheng87 · 2020-10-14T15:10:54Z

Append "Close #12" in git commit message(s). Therefore, you can link a pull request to an issue to show that a fix is in progress and to automatically close the issue when the pull request is merged.
See https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

I have changed commit with key word Close #12 but nothing change. Did it work after PR merge?

jserv · 2020-10-14T15:16:26Z

I have changed commit with key word Close #12 but nothing change. Did it work after PR merge?

If you check the end of #12, the issue is already associated with this pull request. Then, when this pull request is merged, #12 will be closed automatically.

jserv · 2020-10-15T04:55:50Z

README.md

   However, it is valid to use `ptr[0]`, which behaves the same of `*ptr`.
-4. The support of varying number of function arguments is incomplete. No `<stdarg.h>` can be used.
+3. The support of varying number of function arguments is incomplete. No `<stdarg.h>` can be used.


Don't change the order. Minimize the required changes.

Later, we can create the corresponding issues via GitHub rather than the bullet points.

jserv · 2020-10-15T04:59:27Z

src/cfront.c

+        char buffer[10];
+        int isneg = 0;
+        /* global initialization must be constant */
+        /* TODO: support rvalue as expression e.g. int a = 3 + 2; */


This issue should be resolved before this pull request is about to be merged.

eecheng87 · 2020-10-15T14:24:28Z

Latest commit support using expression to initialize global variable (also add test bench in driver.sh).

read_global_assignment using the similar concept with read_expr to process expression.
The reason I don't want merge them in one function is that they actually for totally different purpose.

read_global_assignment need to know the result of expression before execution time and write into .data section before it.
read_expr don't need to get exact result of expression, it only need to know series of instruction and write into .text section. It gets the result during execution time.

jserv · 2020-10-15T16:02:53Z

src/cfront.c

+        error("Invalid value after assignment");
+    lex_expect(T_numeric);
+    if (isneg)
+        return -1 * res;


Rewrite as (-1) * res.

jserv · 2020-10-15T16:03:13Z

src/cfront.c

+    lex_expect(T_numeric);
+    if (isneg)
+        return -1 * res;
+    else


Eliminate this else.

jserv · 2020-10-15T16:04:49Z

src/cfront.c

+        return res;
+}
+
+int get_expression_imm(opcode_t op, int op1, int op2)


The verb get might not reflect what the function does. It can be evaluate or eval in short.

jserv · 2020-10-15T16:05:26Z

src/cfront.c

+        break;
+    default:
+        error("Using operation not support");
+        break;


No break is required. Remove it.

jserv · 2020-10-15T16:07:52Z

README.md

@@ -165,8 +165,7 @@ int fib(int n)      fib:                        Reserve stack frame for function

 1. Any non-zero value is NOT treated as logical truth by all ops.
   That is, the expression `0 == strcmp(ptr, "hello")` is not equivalent to `!strcmp(ptr, "hello")`.
-2. Global variable initialization is not supported. 
-   Therefore, you can not initialize a global such as `int i = [expr]`.
+2. ELF lacks of .bss and .rodata section


Should be "The generated ELF file lacks..."

jserv · 2020-10-16T00:24:30Z

src/cfront.c

+        res = op1 >> op2;
+        break;
+    default:
+        error("Using operation not support");


Should be "The requested operation is not supported."

jserv

Improve the git commit message. You should address 1) how you made it in summary; 2) Known limitation; 3) New test item.

jserv

Change the message "Add correspond test bench in line 250 of driver.sh" to "Add global initialization item in test driver."

jserv

In git commit messages, always use quotes ("...") instead of backquote. Change "help us evaluate" to "evaluates."
Change "This commit doesn't" to "this patch ..."

Close sysprog21#12 1. To initialize global variable, we need to decide the value before execution time. "read_global_assignment" evaluates this immediate value and store it in .data section where store the initial value of variable in generated ELF. 2. This patch doesn't support pointer and array initialization. 3. Add global initialization item in test driver.

jserv · 2020-10-16T05:24:23Z

Thank @eecheng87 for contributing!

jserv requested changes Oct 14, 2020

View reviewed changes

jserv reviewed Oct 14, 2020

View reviewed changes

eecheng87 changed the title ~~Support global variable initialization~~ Support global variable initialization Closes #12 Oct 14, 2020

eecheng87 changed the title ~~Support global variable initialization Closes #12~~ Support global variable initialization Oct 14, 2020

eecheng87 force-pushed the pr branch 2 times, most recently from 1b17da3 to 9e04dcd Compare October 14, 2020 14:35

jserv reviewed Oct 14, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from 7c944b1 to b3c302c Compare October 15, 2020 00:30

jserv reviewed Oct 15, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from e504d73 to 62336d7 Compare October 15, 2020 14:16

eecheng87 requested a review from jserv October 15, 2020 14:26

jserv reviewed Oct 15, 2020

View reviewed changes

src/cfront.c Outdated

lex_expect(T_numeric);

if (isneg)

return -1 * res;

else

Copy link

Collaborator

jserv Oct 15, 2020

Choose a reason for hiding this comment

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

Eliminate this else.

jserv reviewed Oct 15, 2020

View reviewed changes

src/cfront.c Outdated

break;

default:

error("Using operation not support");

break;

Copy link

Collaborator

jserv Oct 15, 2020

Choose a reason for hiding this comment

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

No break is required. Remove it.

jserv reviewed Oct 15, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from 7b49d0e to 429068d Compare October 16, 2020 00:13

jserv reviewed Oct 16, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from d914ce1 to 15481d5 Compare October 16, 2020 00:29

jserv requested changes Oct 16, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from 15481d5 to 21b34b0 Compare October 16, 2020 01:00

jserv requested changes Oct 16, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from 21b34b0 to 41a90d6 Compare October 16, 2020 02:13

jserv requested changes Oct 16, 2020

View reviewed changes

eecheng87 force-pushed the pr branch from 41a90d6 to 12a38bc Compare October 16, 2020 03:12

eecheng87 requested a review from jserv October 16, 2020 03:13

jserv merged commit a53ae96 into sysprog21:master Oct 16, 2020

jserv mentioned this pull request Oct 16, 2020

Simplify function global_init() with global variable initializations #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support global variable initialization #13

Support global variable initialization #13

eecheng87 commented Oct 14, 2020 •

edited

jserv left a comment

jserv commented Oct 14, 2020 •

edited

jserv Oct 14, 2020

jserv Oct 14, 2020

eecheng87 Oct 15, 2020

jserv commented Oct 14, 2020

jserv Oct 14, 2020

eecheng87 Oct 15, 2020 •

edited

jserv Oct 15, 2020

eecheng87 commented Oct 14, 2020

jserv commented Oct 14, 2020

jserv Oct 15, 2020

jserv Oct 15, 2020

jserv Oct 15, 2020

eecheng87 commented Oct 15, 2020 •

edited

jserv Oct 15, 2020

jserv Oct 15, 2020

jserv Oct 15, 2020

jserv Oct 15, 2020

jserv Oct 15, 2020

jserv Oct 16, 2020

jserv left a comment

jserv left a comment

jserv left a comment

jserv commented Oct 16, 2020

Support global variable initialization #13

Support global variable initialization #13

Conversation

eecheng87 commented Oct 14, 2020 • edited

jserv left a comment

Choose a reason for hiding this comment

jserv commented Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Oct 14, 2020

Choose a reason for hiding this comment

eecheng87 Oct 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eecheng87 commented Oct 14, 2020

jserv commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eecheng87 commented Oct 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv left a comment

Choose a reason for hiding this comment

jserv left a comment

Choose a reason for hiding this comment

jserv left a comment

Choose a reason for hiding this comment

jserv commented Oct 16, 2020

eecheng87 commented Oct 14, 2020 •

edited

jserv commented Oct 14, 2020 •

edited

eecheng87 Oct 15, 2020 •

edited

eecheng87 commented Oct 15, 2020 •

edited