i#6585: Add drcachesim vector length trace marker #6603
Conversation
Adds a new trace marker
TRACE_MARKER_TYPE_DYNAMIC_VECTOR_LENGTH
to drcachesim that indicates the current vector length for architectures
which have a dynamic vector length that can't be statically determined
from the instruction.
The marker is emitted as part of the thread header when running on AArch64
with SVE support, but in the future could also be used to track changes
in the vector length after prctl(PR_SVE_SET_VL, ..) system calls.
Some SVE load and store instructions such as
``` LDR <Zt>, [<Xn|SP>{, #<imm>, MUL VL}]```
or
``` ST1D { <Zt>.D }, <Pg>, [<Xn|SP>{, #<imm>, MUL VL}]```
scale the immediate offset based on the hardware vector length so
knowing the correct vector length for the traced application is
important to properly decode and analyse these instructions.
Fixes: #6585
Change-Id: Iae665a82d689419fa345717bab3f2f304cd57ec3
Change-Id: I3ebefbd67c84c8f0ca3f68a63d1b88fe8a2554a2
| // sve_veclen in libdynamorio, but not the one in drcachesim. | ||
| // Unfortunately it is the version of sve_veclen in drcachesim that gets used when | ||
| // decoding in raw2trace so we need to explicitly initialize its sve_veclen here. | ||
| dr_set_sve_vector_length(proc_get_vector_length_bytes() * 8); |
There was a problem hiding this comment.
If someone processes an older trace without the new marker, no one will set the length and it will be the default which seems to be 0? If 0, what does the decoder do: does it error out? Should there be a better default?
There was a problem hiding this comment.
Outside of drmemtrace, this seems to make using DR's standalone decoder painful for SVE: if the default is 0 would someone decoding SVE have the decoder fall over? Anyone using the decoder now has to call the set vector length function? Should that be documented?
There was a problem hiding this comment.
If someone processes an older trace without the new marker, no one will set the length and it will be the default which seems to be 0? If 0, what does the decoder do: does it error out? Should there be a better default?
Currently, if the vector length is set to 0 the decoder will not produce an error. It will just use 0 in its calculations which will result in some SVE load/store instructions being decoded with a 0 byte offset when they shouldn't be. drdisas has a -vl command line option to set the vector length. We could add an equivalent option to drcachesim to allow users to specify a vector length for traces which don't contain a vector length marker.
Outside of drmemtrace, this seems to make using DR's standalone decoder painful for SVE: if the default is 0 would someone decoding SVE have the decoder fall over? Anyone using the decoder now has to call the set vector length function? Should that be documented?
In order to handle vector length changes properly in DynamoRIO (#6625) I think we are going to have to change the way vector length is stored and managed, so that might mean changes to dr_set_sve_vector_length(). We can make sure as part of that issue that how it works, and any requirements on standalone decoder users are properly documented.
There was a problem hiding this comment.
Probably the default should be 128 or something valid so that reasonable results are obtained without explicit calls to set vector lengths: I suspect 99% of users are not going to make that call on the first pass and a reasonable default should be provided.
There was a problem hiding this comment.
so that reasonable results are obtained without explicit calls to set vector lengths:
Explicit failures may be better than hidden trace/decoding correctness issues.
a reasonable default should be provided.
Instead of a fixed number, could we initialize it using proc_get_vector_length_bytes in the decoder? E.g.,
dynamorio/core/ir/opnd_shared.c
Line 2762 in 62d441e
If it cannot be obtained from the environment, it may be better to fail, so that the user can set it explicitly. (except in cases where we know a certain number to be a very good default)
There was a problem hiding this comment.
Having a decoder that fails when you try to use if you don't make a call to set the vector length does not sound user-friendly to me: most will not know they're supposed to call that; many will not care and just want to see the opcode in offline decoding. Some kind of general initialization routine seems better; default also seem good to me.
Let's file a new issue on this. It relates to several other issues on trying to make drdecode work without explicit initialization, which maybe we have to abandon for other reasons. I will file one and put in the xrefs.
| // sve_veclen in libdynamorio, but not the one in drcachesim. | ||
| // Unfortunately it is the version of sve_veclen in drcachesim that gets used when | ||
| // decoding in raw2trace so we need to explicitly initialize its sve_veclen here. | ||
| dr_set_sve_vector_length(proc_get_vector_length_bytes() * 8); |
There was a problem hiding this comment.
nit: too late to fix I know: the description has the wrong name TRACE_MARKER_TYPE_DYNAMIC_VECTOR_LENGTH
Adds a new trace marker
TRACE_MARKER_TYPE_DYNAMIC_VECTOR_LENGTHto drcachesim that indicates the current vector length for architectures which have a dynamic vector length that can't be statically determined from the instruction.
The marker is emitted as part of the thread header when running on AArch64 with SVE support, but in the future could also be used to track changes in the vector length after prctl(PR_SVE_SET_VL, ..) system calls (i#6625).
Some SVE load and store instructions such as
LDR <Zt>, [<Xn|SP>{, #<imm>, MUL VL}]or
ST1D { <Zt>.D }, <Pg>, [<Xn|SP>{, #<imm>, MUL VL}]scale the immediate offset based on the hardware vector length so knowing the correct vector length for the traced application is important to properly decode and analyse these instructions.
Fixes: #6585
Issues: #6625