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

Move some macros used in .asm files into header files. #7202

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

egrimley-arm
Copy link
Contributor

@egrimley-arm egrimley-arm commented Jan 21, 2025

The macros are defined in asm_offsets.h and the values are checked at compile time by asm_offsets.cpp.

A few other changes were needed to make this work:

  • Some declarations are moved from mangle.c into a new file mangle.h.

  • A declaration is moved from module_elf.c to module_private.h.

  • A preprocessor check in opnd_api.h dating back to 2010 is removed.

  • A small change in stats.h for compatibility with C++11.

The macros are defined in offsets.h and the values are checked
at compile time by offsets.c.

Change-Id: I0a22ae5c0046769dc3fb763db8904d52530e48af
@egrimley-arm
Copy link
Contributor Author

One could use a helper program on the host to automatically generate the header files that define the offsets, but that would be much more complex to implement and an ongoing maintenance burden in the build system. Since these offsets are very rarely changed, and any change of them will typically involve simultaneous intricate changes to assembly and code that generates assembly I don't think it makes sense to generate the header files automatically. Better just to check the values with something like this approach.

One could use the preprocessor to make the source slightly simpler in a way, but it would then be much more complex in another way, and the error message when an offset is incorrect would be less readable, I think. Currently the error message looks like this:

.../dynamorio/core/arch/aarch64/offsets.c:9:9: error: size of array ‘x1’ is negative
    9 |     int x1[dcontext_t_OFFSET_dstack == offsetof(dcontext_t, dstack) ? 1 : -1];

I think that makes perfect sense to a programmer, and, if not, there's the comment preceding it in the source.

Another option would be to generate an error message at run time. You would then have total control over what the message looks like, but an error at build time seems better.

And using static_assert is certainly worth considering. I'm a bit confused about which versions of static_assert (or _Static_assert or whatever) are available with which versions of which compilers, and which header files need to be included to make them work, so perhaps for DynamoRIO it's better to use something totally portable like this array length trick.

core/arch/aarch64/offsets.c Outdated Show resolved Hide resolved
core/CMakeLists.txt Outdated Show resolved Hide resolved
core/arch/aarch64/offsets.h Outdated Show resolved Hide resolved
Change-Id: If271ed6c8c97ee5e636d2d7c2b012898812cbc28
Move some macros used in .asm files into header files.

The macros are defined in offsets.h and the values are checked
at compile time by offsets.cpp.

Change-Id: If903c6cb8128ca955979eaba5c2d6eaf68374259
Change-Id: If0598cd9327ce075f77c20839dd9bad312a9f96e
Change-Id: Ibbd01c543f8a162557640142d488efd63589d2f2
Change-Id: I300e728dac8d4ca30c72d1b12f366b91c007fa7d
Change-Id: I008235196859c32ec4c2a9891cdbb4e9a6f63d2f
Change-Id: Ie574980db7deb75db6647005443915828147d274
@egrimley-arm
Copy link
Contributor Author

Does this (https://github.com/DynamoRIO/dynamorio/actions/runs/12928500203/job/36055907604?pr=7202) make any sense to anyone?

aarch64-on-x86: **** 6 build errors ****
	/.../dynamorio/core/unix/../ir/opnd_api.h:47:6: error: #error REG_ enum conflict between DR and ucontext.h! Use DR_REG_ constants instead.
	make[2]: *** [core/CMakeFiles/dynamorio.dir/build.make:1021: core/CMakeFiles/dynamorio.dir/arch/aarch64/asm_offsets.cpp.o] Error 1

I don't understand how it's getting from #include "../globals.h" to including opnd_api.h, and there are other files which start by including globals.h. The error doesn't happen with my toolchain so it's hard for me to investigate.

"#error REG_ enum conflict between DR and ucontext.h! Use DR_REG_ constants instead."

Change-Id: I06cea0f27cc0cd817cd40228350ec2303e91d53f
It was added by 96cfabc in Oct 2010 and might not be useful any more?

Change-Id: I89564eca250bd8e12745f0523874ec8b0807722f
It was also necessary to move some declarations around.

Change-Id: Ib4c0419ae8332150a7b3cc476840384cf83e9e0e
Change-Id: Ie0d7d560a9e3579153e940f5cad851f442a02d6e
Change-Id: Ic1d3496ae991945c0666f8b2b026613d58872086
Change-Id: I4ae09c88f13a4778c09689cce5d5ac0a07762e4f
core/arch/aarch64/asm_offsets.h Show resolved Hide resolved

#define CHECK(x) static_assert(x, "macro in asm_offsets.h defined incorrectly")

CHECK(dcontext_t_OFFSET_dstack == offsetof(dcontext_t, dstack));
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of this looks like it should be generated using the "foox.h" pattern used in many other parts of DR (optionsx.h, statsx.h, kstatsx.h, etc.):

We'd have:

asm_offsetsx.h

OFFSET(spill_state_t, r1, 8)

arm_offsets.cpp

#define OFFSET(struct, field, value) CHECK(value == offsetof(struct, field));
#include "asm_offsetsx.h"

But I'm not sure it's possible to get arm_offsets.h to generate a literal without running some other script first (we'd try to use cmake to avoid pulling in more deps) -- so then the only win would be putting all the stuff you need to edit in the same place, which might still be worth it:

asm_offsetsx.h

OFFSET(spill_state_t, r1, 8)
#define spill_state_t_OFFSET_r1 8

arm_offsets.cpp

#define OFFSET(struct, field, value) CHECK(value == offsetof(struct, field));
#include "asm_offsetsx.h"

Now the only file a human edits is asm_offsetsx.h. Hmm. Worth it?

At least add a XXX comment about auto-generating via asm_offsetsx.h plus a script or a full script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a good idea. I'll try it.

core/CMakeLists.txt Show resolved Hide resolved
@@ -42,11 +42,6 @@
* @brief Functions and defines to create and manipulate instruction operands.
*/

/* Catch conflicts if ucontext.h is included before us */
#if defined(DR_REG_ENUM_COMPATIBILITY) && (defined(REG_EAX) || defined(REG_RAX))
# error REG_ enum conflict between DR and ucontext.h! Use DR_REG_ constants instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a useful check: I don't think it should just be thrown away. If you hit it, that means there is a real enum conflict, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. It happened on the CI but I couldn't reproduce it locally. If I'm lucky the problem will go away when I stop using C++.

core/stats.h Show resolved Hide resolved
core/unix/module_private.h Show resolved Hide resolved
core/arch/aarch64/mangle.h Show resolved Hide resolved
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.

2 participants