-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Implement current_dll_path for AIX #109522
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
Conversation
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
#[cfg(target_os = "aix")] | ||
unsafe { | ||
let addr = current_dll_path as u64; | ||
let mut buffer = vec![0i8; 4096]; |
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 may not be properly aligned for ld_info
(it probably will in practice but we shouldn't rely on it). You should use a vec![std::mem::zeroed::<libc::ld_info>(); N]
instead (and make sure to properly translate the length when calling loadquery
).
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.
That's a good point.
You could use MaybeUninit as a field in an aligned struct instead. With the alignment is the same as ld_info's. |
r? @Nilstrieb |
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.
Thanks for adding the extra context around function descriptors, I wasn't aware of that.
After addressing my comment, have you tested the change? We can't really add a test ourselves and I don't have access to an AIX machine, but it would nice if you could test it to make sure that it works.
loop { | ||
let text_base = (*current).ldinfo_textorg as u64; | ||
let text_end = text_base + (*current).ldinfo_textsize; | ||
if (text_base..text_end).contains(&addr) { |
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.
The function descriptor lives in the data and not text segment, doesn't it? So we should look in there instead of text. (I wasn't aware of AIX function descriptors when I made the first comment asking why it would be in data).
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.
Yes, the function descriptor lives in the data segment. In this context, the correctness should be the same for either using text or data segment. For sure, I'll update to use data segment which is less surprising than deferencing from the function descriptor.
Yes, I tested it via bootstrapping stage2 rustc. |
eb122bf
to
49f63eb
Compare
@bors r+ |
…mpiler-errors Rollup of 6 pull requests Successful merges: - rust-lang#109347 (Skip no_mangle if the item has no name.) - rust-lang#109522 (Implement current_dll_path for AIX) - rust-lang#109679 (Freshen normalizes-to hack goal RHS in the evaluate loop) - rust-lang#109704 (resolve: Minor improvements to effective visibilities) - rust-lang#109739 (Closures always implement `FnOnce` in new solver) - rust-lang#109758 (Parallel compiler cleanups) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
AIX doesn't feature
dladdr
, useloadquery
instead.loadquery
is documented in https://www.ibm.com/docs/en/aix/7.2?topic=l-loadquery-subroutine.