Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix missing header and incorrect asm #84

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

dzhang20
Copy link
Contributor

Module:
fix missing header and incorrect asm instruction
Description:
Latest update on master branch introduce few of the issues that will prevent users from successfully building the code when running "s2e update && s2e build". The issues are detailed below:

  1. the errno is used without correctly including the errno.h header in libtcg/src/utils/memalign.c.
  2. In guest/s2ebios/tests/tests64.asm, the use of lea instruction is in an incorrect way that will cause compilation issue. lea (Load Effective Address) instruction in assembly language allows you to perform arithmetic operations while moving data. It's typically used to calculate the addresses of variables. However, in the test file, it's used as lea rcx, 0x20000 in few of the places. Thus, I tentatively replaced lea with mov (waiting for authors approval).

Signed-off-by: Ding Zhang [email protected]

@vitaly-cyberhaven
Copy link
Contributor

This looks much easier to review, thanks for re-submitting. Any reason you couldn't reopen the old PR and force-push there? That would help keep track of comments in one place.

A couple more things:

  • Split the commit into two: the header fix and the asm fix. They are unrelated and should be separate.
  • Prefix the commit message appropriately (see git history for examples).

@dzhang20 dzhang20 force-pushed the fix_header_and_asm_test branch 4 times, most recently from b610831 to b8b78d9 Compare September 26, 2023 16:23
@dzhang20
Copy link
Contributor Author

This looks much easier to review, thanks for re-submitting. Any reason you couldn't reopen the old PR and force-push there? That would help keep track of comments in one place.

A couple more things:

  • Split the commit into two: the header fix and the asm fix. They are unrelated and should be separate.
  • Prefix the commit message appropriately (see git history for examples).

Sorry, the previous branch with the old pr is deleted and one new branch with the same name is then being created by another user. Thus, I think it's easier to reissue another pr in a clean state.
I have update the pr with the changes:

  1. split commit into two
  2. prefix the commit message

Best

@vitalych
Copy link
Member

vitalych commented Sep 26, 2023

Please rebase on master instead of merging master into the branch. I see a "Merge branch master" commit. These should not be in the PR.

@dzhang20 dzhang20 force-pushed the fix_header_and_asm_test branch from 7fd7d4c to 9127ca5 Compare September 26, 2023 16:48
@dzhang20
Copy link
Contributor Author

Please rebase on master instead of merging master into the branch. I see a "Merge branch master" commit. These should not be in the PR.

Done

@@ -53,31 +53,31 @@ test_memory_rw_new_page:

; 1 byte rw
mov al, 0xdf
lea rcx, 0x20000
mov rcx, 0x20000
Copy link
Member

Choose a reason for hiding this comment

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

nasm on my system doesn't throw any error on this. I am curious what's on yours.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It throws error: invalid combination of opcode and operands

@vitalych
Copy link
Member

You also need to run clang format (see failing test)

@dzhang20 dzhang20 force-pushed the fix_header_and_asm_test branch from 9127ca5 to aa7c142 Compare September 26, 2023 21:56
@dzhang20 dzhang20 force-pushed the fix_header_and_asm_test branch from aa7c142 to 59a1cc2 Compare September 26, 2023 21:58
@dzhang20 dzhang20 requested a review from vitalych September 26, 2023 22:01
@vitalych vitalych merged commit 18d7fe5 into S2E:master Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants