-
Notifications
You must be signed in to change notification settings - Fork 584
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
Support maximum of 64kB page on aarch64 #3146
Conversation
c433c07
to
26e5e06
Compare
38934bb
to
1b8a024
Compare
5958ebd
to
e54fe7d
Compare
|
4f47bcd
to
d48aa8e
Compare
.set preload_thread_locals,0x70001000 | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this use 0x70000000 + RR_PAGE_SIZE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was actually just because I was too lazy to check if the macro was defined by this point ;-p...
#define RR_THREAD_LOCALS_PAGE_ADDR 0x70001000 | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise this could be RR_PAGE_ADDR + page_size()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the test so it's somewhat different. It would have to be rr_page_size()
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean librrpreload.so, not librrpage.so?
I think we should move rr_page_size()
to to util.h
/util.cc
as a global function that's a peer of page_size()
. I think we should also change its name ... how about rr_max_supported_page_size()
since it's the the maximum page size supported by this build of rr (which is baked into librrpreload.so layout)?
d48aa8e
to
472e9c7
Compare
I'm fine with renaming. I pick the current name mainly to emphasize that it's the size of the block in the preload library, since that's actually what most of the code uses it for (they use that size with a multiplication factor to compute an offset/address). It's somewhat less obvious what |
I also realized that I left one instance of RR_PAGE_SIZE unchanged so I'll need to fix that... I also did only minimum amount of refactoring but I could refactor it more to make most things that need a page size to get them from fewer places. I wasn't sure how much to abstract out/assume but I guess I'll just assume that we'll have an architecture dependent maximum page size to support and refactor everything around that. If there's an architecture that doesn't have a upper limit for page size it might be a problem but I think the system linker would also have to make such a decision anyway..... |
OK how about calling it |
472e9c7
to
5806c1e
Compare
Is there any specific reason to have both a function version |
You're right. We should just have a |
OK, so there's no need to make it |
This has to be a build-time property because it determines the layout of the preload library. |
But in principle it only need to be a build time property Anyway, constant it is for now... |
Right. I want it in the trace header so that later we can change the value for a given architecture and extend rr to support replaying "old" traces with a different value, so it would have to be a function outside the preload library. But we don't need to do that now. |
Just to be consistent on what we use
To better reflect what we use this for. This is the size of the blocks (pages) within librrpage.so and we use this size mostly for computing addresses within/around that file.
PRELOAD_LIBRARY_PAGE_SIZE is the size/alignment of librrpage.so which is determined at compile time. It should not depend on the runtime system page size.
Use a cmake variable to control the page size/alignment used by the linker script and the tweak script.
…ZE in the trace In case we need to retrieve this information in a future version.
Build the preload library to be aligned to 64kB following the default of binutils. Fix rr-debugger#3143
5806c1e
to
088a501
Compare
I updated with the new name and replaced all use of rr_page_size with the constant. As shown in the last commit, there are still a few (5) places where this value is defined. This is because, AFAICT, these do not include a shared header. The simplest method to avoid this is to just add a |
(The new field in the trace file header is also updated) |
I think this is good enough for now. |
Actually I added a followup patch 9d623e8 to set the default page size to its correct value, and made the page sizes UInt32. |
Build the preload library to be aligned to 64kB following the default of binutils.
I've never used capnp before and I'm not sure yet how to add and use a new header field.....
If I'm doing things correctly, this should not affect x86...
Fix #3143