DynamoRIO / DynamoRIO/dynamorio

[Feature request] Allow MAX_FRAGMENT_SIZE to take on values greater than USHRT_MAX

Open
#4,707 5 comments 0 reactions 0 assignees View on GitHub
Priority-Low Type-Feature
Dominant language
C
Stars
3.2k
Forks
629
Avg merge
2d 18h
Merged PRs (30d)
30

Description

Hi again,

I've been developing a fairly heavy-duty instrumentation client that, among other things, wraps and dumps all memory load and store instructions, à la memtrace_simple. Recently, I compiled the SPEC CPU2017-rate suite for x86-64 with gcc 10.2 at `-O3`, and saw the following on `cam4_r`, `parest_r`, and `wrf_r`:

`Basic block or trace instrumentation exceeded maximum size. Try lowering -max_bb_instrs and/or -max_trace_bbs.`

I ran DR with `-debug -loglevel 3`, and discovered that, as expected, gcc was doing (an admittedly incredible amount of) inlining and loop unrolling to produce a basic block w/instrumented size exceeding 64KiB.

So, it seems like alternatives at this point are either to lower `-max_bb_instrs/-max_trace_bbs`, which entails missing a large and relevant portion of the workload, or to increase `MAX_FRAGMENT_SIZE` (and probably increase `-max_bb_instrs/-max_trace_bbs` after that) in order to support larger fragments.

Currently, MAX_FRAGMENT_SIZE is maxing out the size of the ushort fragment size field. I took a rudimentary stab at changing this and all related fields from ushort to uint, by just grepping through the codebase. I've attached a patch that attempts to do this, at least for just Linux: [max_fragment_size_diff.txt](https://github.com/DynamoRIO/dynamorio/files/5921258/max_fragment_size_diff.txt). There's something wrong, though, as the workloads progress well past the point where they asserted out previously, but throw a meta-instruction fault assertion later on.

On one hand, this is a pretty heavyweight use case. On the other hand, the trend of aggressive inlining probably isn't going away, and two extra bytes per fragment (?) for a larger size field seems like a small amount of overhead.

Any thoughts? And happy to help any way I can. Thanks!

Contributor guide

Open the contributing guide

Research direction

The issue centers on MAX_FRAGMENT_SIZE, related ushort fields, and the attached max_fragment_size_diff.txt patch; start by reviewing those changes and the later meta-instruction fault assertion. Reproduce with the SPEC CPU2017-rate cases cam4_r, parest_r, and wrf_r, and consider the work done when larger fragments run without the original size error or later assertion.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, linux
Domain
devtools
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.