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.

Sign up for GitHub

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

Jump to bottom

s390x: update to upstream revision 7e9113cb7 #25

Closed
wants to merge 9 commits into from
Closed

s390x: update to upstream revision 7e9113cb7 #25

wants to merge 9 commits into from

Conversation

mephi42
Copy link
Contributor

@mephi42 mephi42 commented Mar 17, 2019

The old endianness patch has finally went upstream, and so I can now import all the sweet modern s390x insns. There are a bunch of pre-req changes in the common VEX code that need to be picked as well, but as far as I can tell they don't negatively affect other architectures.

In order for the whole thing to work, archinfo and pyvex need to be adapted. I will post the corresponding pull requests shortly.

@mephi42
Pick common code changes from 94d63e3
4a82a0e 
94d63e3: Add a mechanism for hinting to the core disassembler loop
@mephi42
Port common code changes from efa1e5e
d57d2ac 
efa1e5e: VEX register allocator version 3
@mephi42
Port common code changes from 83cabd3
66be59d 
83cabd3: Refactor tracking of MOV coalescing
@mephi42
Pick common code changes from f1a49ee
4731397 
f1a49ee: s390x: z13 vector "support" instructions not implemented
@mephi42
Pick common code changes from d44563c
7e18b5a 
d44563c: s390x: new non-vector z13 instructions not implemented
@mephi42
Pick common code changes from 20976f43
fa32291 
20976f43: s390x: Implement conditional trap instructions
@mephi42
Pick common code changes from 1cc1d564
4fee547 
1cc1d564: s390x: Vector integer and string instruction support
@mephi42
s390x: update to upstream revision 7e9113cb7
84adf26 
- Update genoffsets
mephi42 added a commit to mephi42/archinfo that referenced this pull request Mar 17, 2019
@mephi42
Adapt to s390x valgrind update
e67f7d2 
angr/vex#25
@mephi42 mephi42 mentioned this pull request Mar 17, 2019
Merged
mephi42 added a commit to mephi42/pyvex that referenced this pull request Mar 17, 2019
@mephi42
Adapt to s390x valgrind update
ce8d1e8 
angr/vex#25
@mephi42 mephi42 mentioned this pull request Mar 17, 2019
Merged
@zardus
Copy link
Member

zardus commented Mar 17, 2019 via email

@mephi42
Copy link
Contributor Author

mephi42 commented Mar 17, 2019

I agree that rebasing approach would be nicer, since it would make updates like this one trivial. I have actually tried doing it when adding the initial s390x support, but ran into considerable amount of merge conflicts. If memory serves me right, one of the bigger sources was msvc support. Maybe it might be worth contributing upstream anyway?

@zardus
Copy link
Member

zardus commented Mar 26, 2019

MSVC would be awesome to upstream, but given that the valgrind people mostly view VEX as a valgrind tool, and Windows isn't a target for them, they might not take the patches. Worth a shot, though.

@twizmwazin, this one is a doozie, but could you look into rebasing our current VEX changes on modern valgrind? This might be quite a project, but let's at least keep it in mind.

@rhelmot
Copy link
Member

rhelmot commented Mar 26, 2019

Here's what julian said re: the msvc patch when I submitted them originally back in 2017:

Big, complex patch. Need to come back to it. It would be easier to review and
discuss if you could break it into smaller pieces, by feature. Eg, break out
the case-range removal since that's uncontroversial and easy to land. (But
please .. preserve the hex casing: "case 0xFD .. 0xFF:" -> "case 0xFD: etc" and
not "case 0xfd: etc")

Does MSVC really disallow empty structs? Having to put "Int nop;" all over
the place kinda sucks.

The problem with rebasing at this point is that the valgrind and vex repos have been merged. There might be some git wizardry we can do to mirror only a subset of a repo, but like, yikes.

The main thing that was asked when we did the first round of upstreaming was that we needed tests, and proof that it won't break valgrind. I do have a test harness for how to add vex-only tests to modern valgrind, so whoever wants to do this should ping me for that. We'll need to port our pyvex tests to C, but that's it.

@rhelmot
Copy link
Member

rhelmot commented Mar 28, 2019

I think I would like to merge this as is now and deal with rebasing later. @mephi42, can you run the angr/pyvex/claripy/cle tests manually? I don't trust our CI system to build vex...

We can deal with the bad git repo situation the same way we did when it was svn: we just copy the base code wholesale and call it an initial commit.

@twizmwazin
Copy link
Member

@rhelmot I am looking into rebasing, there are a few changes there. First, it is now in one repo with the rest of valgrind. One of the consequences of this is that the build system is more integrated, and I am unsure to what extent they support building just the vex library.

For a long term solution, dumping the subtree here doesn't make much sense, we will end up back in the same place. Instead, it is probably best to just have a fork of the valgrind repo with the necessary build system changes and patches that can be fairly easily rebased on a regular basis, like after each upstream stable release. Additionally, for the purpose of making our lives easier, it would be in out best interest to regularly see how much can be upstreamed, to keep our patch est minimal.

None of this really interferes with this PR being merged, just something to think about on the side.

@rhelmot
Copy link
Member

rhelmot commented Mar 28, 2019

here is my fork of valgrind with progress on being able to build and test vex standalone.

@mephi42
Copy link
Contributor Author

mephi42 commented Apr 26, 2019

I managed to run the tests as follows (tests/test.sh did not work, because it did not change working directories, and all relative paths in tests were messed up):

angr-dev$ docker build . -t angr
angr-dev$ docker run -it --rm -v "$PWD:$PWD" -w "$PWD" angr bash
# . /usr/local/bin/virtualenvwrapper.sh
# workon angr
# for project in ailment angr angrop archinfo claripy cle pyvex; do echo "$project"; (cd "$project/tests" && NOSE_PROCESSES=$(nproc) NOSE_PROCESS_TIMEOUT=600 NOSE_PROCESS_RESTARTWORKER=1 nosetests) >"$project.out" 2>"$project.err"; done

I ran into the following errors in various tests, all of which appear to be pre-existing:

  File "/home/builder/angr-dev/angr/tests/test_concrete_not_packed_elf32.py", line 2, in <module>
    import avatar2
ModuleNotFoundError: No module named 'avatar2'

  File "/home/builder/angr-dev/angr/angr/analyses/reaching_definitions/engine_ail.py", line 100, in _ail_handle_Store
    l.info('Data to write at address %#x undefined, ins_addr = %#x.', a, self.ins_addr)
  File "/usr/lib/python3.6/logging/__init__.py", line 1308, in info
    self._log(INFO, msg, args, **kwargs)
  File "/usr/lib/python3.6/logging/__init__.py", line 1444, in _log
    self.handle(record)
  File "/usr/lib/python3.6/logging/__init__.py", line 1454, in handle
    self.callHandlers(record)
  File "/usr/lib/python3.6/logging/__init__.py", line 1516, in callHandlers
    hdlr.handle(record)
  File "/usr/lib/python3.6/logging/__init__.py", line 865, in handle
    self.emit(record)
  File "/root/.virtualenvs/angr/lib/python3.6/site-packages/nose/plugins/logcapture.py", line 82, in emit
    self.buffer.append(self.format(record))
  File "/usr/lib/python3.6/logging/__init__.py", line 840, in format
    return fmt.format(record)
  File "/usr/lib/python3.6/logging/__init__.py", line 577, in format
    record.message = record.getMessage()
  File "/usr/lib/python3.6/logging/__init__.py", line 338, in getMessage
    msg = msg % self.args
TypeError: %x format: an integer is required, not SpOffset

  File "/home/builder/angr-dev/claripy/claripy/smtlib_utils.py", line 24, in expect_assignment_tuple
    self.expect('(')
  File "/home/builder/angr-dev/claripy/claripy/smtlib_utils.py", line 18, in expect
    t = self.tokens.consume()
  File "/root/.virtualenvs/angr/lib/python3.6/site-packages/pysmt/smtlib/parser/parser.py", line 192, in consume
    return self.extra_queue.pop(0)
TypeError: pop() takes no arguments (1 given)

Overall stats:

ailment: Ran 5 tests in 3.792s
angr: Ran 446 tests in 1017.093s FAILED (errors=21, failures=1)
angrop: Ran 4 tests in 165.625s
archinfo: Ran 0 tests in 0.862s
claripy: Ran 467 tests in 19.402s FAILED (SKIP=237, errors=16)
cle: Ran 49 tests in 12.059s
pyvex: Ran 51 tests in 32.655s

@twizmwazin twizmwazin mentioned this pull request May 10, 2019
Merged
@mephi42
Copy link
Contributor Author

mephi42 commented Jun 19, 2019

I made another update (on top of existing ones for simplicity), which brings support for several z14 instructions.

mephi42 added a commit to mephi42/pyvex that referenced this pull request Dec 2, 2019
@mephi42
Adapt to s390x valgrind update
e0d694f 
angr/vex#25
mephi42 added a commit to mephi42/archinfo that referenced this pull request Dec 2, 2019
@mephi42
Adapt to s390x valgrind update
2371aac 
angr/vex#25
@mephi42
Copy link
Contributor Author

mephi42 commented Dec 2, 2019

I've rebased the code and pushed it to #26 (didn't want to split this time, so I'll close this PR).

I've also rebased the associated archinfo and pyvex PRs:

angr/archinfo#61
angr/pyvex#186

Could you have a look please?

@mephi42 mephi42 closed this Dec 2, 2019
rhelmot pushed a commit to angr/pyvex that referenced this pull request Jan 24, 2020
@mephi42 @rhelmot
Adapt to s390x valgrind update
e7cf6e7 
angr/vex#25
rhelmot pushed a commit to angr/archinfo that referenced this pull request Jan 24, 2020
@mephi42 @rhelmot
Adapt to s390x valgrind update
a078551 
angr/vex#25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhelmot rhelmot rhelmot approved these changes

Assignees

@twizmwazin twizmwazin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mephi42 @zardus @rhelmot @twizmwazin