Add AIX support in gimli symbolizer #508

bzEq · 2023-01-12T09:06:33Z

No description provided.

ecnelises · 2023-04-27T06:27:39Z

Do we need to upgrade object to 0.31.0 and enable xcoff feature? (there's one upgrading it #522 )

workingjubilee · 2023-06-22T01:54:37Z

That's weird. More of the tests should be not-failing at this point. Can you rebase this on master? The object bump is merged, for note.

bzEq · 2023-06-28T08:17:52Z

Rebased and fixed build failures.

workingjubilee

Thanks for the rebase! Is there a version of cargo that can build this while cross-compiling? With -Zbuild-std, perhaps?

This review is not exhaustive! Rather, having appropriate connections drawn would make it much easier to properly review and, most importantly, maintain, so later updates to this crate don't accidentally burn down all your hard work.

src/symbolize/gimli.rs

src/symbolize/gimli/xcoff.rs

tests/smoke.rs

workingjubilee · 2023-06-29T03:24:09Z

src/symbolize/gimli/libs_aix.rs

+        loop {
+            if libc::loadquery(
+                libc::L_GETINFO,
+                buffer.as_mut_ptr() as *mut i8,


Is this standing in for c_char? I would prefer the type was annotated appropriately, if so. I don't really care about other "ctypes", but c_char is special in C so I prefer to be pedantic about that particular one.

The prototype of loadquery in C is

int loadquery(int Flags, void *Buffer, unsigned BufferLength);

Usually, we allocate a char * buffer in C and pass it to loadquery.
Are you implying we should modify the type in libc crate?

Oh.
It should probably have been *mut c_void in the libc bindings, then.
You know that the libc crate's stability policy means that you effectively cannot change the bindings in the libc crate, right? It does breaking changes only very, very rarely, and considers virtually everything to be breaking if it could possibly at all be breaking. So you should probably avoid committing unnecessary platform-specific bindings to it.

src/symbolize/gimli/libs_aix.rs

src/symbolize/gimli/xcoff.rs

src/symbolize/gimli/libs_aix.rs

workingjubilee · 2023-06-29T04:28:50Z

Also I realized my initial review includes a lot of specific questions about segments in the code and what the supporting concepts are for explaining them, but you don't need to address all of those questions in such a point-by-point manner if you want to take a more high-level approach to documenting (or pointing to existing documentation of) the concepts in question.

bzEq · 2023-07-05T02:18:39Z

you don't need to address all of those questions in such a point-by-point manner if you want to take a more high-level approach to documenting (or pointing to existing documentation of) the concepts in question.

I'll try my best to give an overview of AIX ABI. Since the latest AIX ABI is not formally documented, its behaviors largely depend on assembler manual and current implementation of IBM OpenXL/XL.

workingjubilee · 2023-07-05T04:52:27Z

Ah, I had a feeling it might be something like that. That unfortunately motivates my questions more, since we can't easily look up things and instead have to piece together answers from the scraps we can dig up. But I'm somewhat familiar with typical Power ISA assembly, at least, so this assembler manual is very helpful.

bzEq · 2023-07-25T02:05:11Z

Gentle ping.

workingjubilee

Took a new look. Thank you for addressing the prior round of review, having scanned the documentation a bit I feel relatively confident we can land this once a few last questions and cleanups are done.

tests/smoke.rs

src/symbolize/gimli.rs

src/symbolize/gimli/libs_aix.rs

workingjubilee · 2023-08-18T01:13:42Z

src/symbolize/gimli/libs_aix.rs

+            ) != -1
+            {
+                break;
+            } else {
+                if Error::last_os_error().raw_os_error().unwrap() != libc::ENOMEM {
+                    return Vec::new();
+                }
+                buffer.resize(buffer.len() * 2, mem::zeroed::<libc::ld_info>());
+            }


I would prefer this consistently uses if-else-if nesting or consistently use "rely on earlier break/return" instead of this crossing-over between the two styles. My understanding of this control flow is each of these expressions can only be reached if all conditions before them fail to be met, just like in an if-else-if-else chain, so there is no need for the early return to have its own if-statement, unless the entire set is "flat".

I do realize they are conceptually separate but that also means the break; doesn't need an else after it.

workingjubilee · 2023-08-18T01:17:31Z

src/symbolize/gimli/libs_aix.rs

+                if Error::last_os_error().raw_os_error().unwrap() != libc::ENOMEM {
+                    return Vec::new();
+                }
+                buffer.resize(buffer.len() * 2, mem::zeroed::<libc::ld_info>());


Okay, my question re: why the control flow is like this also becomes: is there a reason this unwraps instead of using something like

Suggested change

if Error::last_os_error().raw_os_error().unwrap() != libc::ENOMEM {

return Vec::new();

}

buffer.resize(buffer.len() * 2, mem::zeroed::<libc::ld_info>());

match Error::last_os_error().raw_os_error() {

Some(libc::ENOMEM) => buffer.resize(buffer.len() * 2, mem::zeroed::<libc::ld_info>()),

Some(libc::EINVAL | libc::EFAULT) => /* something? */,

_ => unreachable!(),

}

When reasoning by negation and positive reasoning are equally available, I prefer to avoid negation.

src/symbolize/gimli/xcoff.rs

workingjubilee · 2023-08-18T01:42:53Z

src/symbolize/gimli/xcoff.rs

+        for member in archive.members() {
+            if let Ok(member) = member {
+                let name = OsStr::from_bytes(member.name());
+                if name == member_name {


Same remarks regarding eliminating rightward drift as earlier.

src/symbolize/gimli/xcoff.rs

workingjubilee · 2023-08-18T02:15:15Z

Also feel free to squash/rebase this with your next pass of changes to however the commit history should look.

- Use `loadquery` to get loaded libraries - Use object crate to parse XCOFF and get image information

bzEq · 2023-08-23T08:28:18Z

Addressed comment and rebased. Thanks @workingjubilee for great suggestions on coding style.

github-actions · 2023-08-23T08:30:18Z

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 533920B
Updated binary size: 530744B
Difference: -3176B (-0.59%)

bzEq · 2023-09-08T23:31:14Z

Gentle ping.

workingjubilee · 2023-09-09T01:34:03Z

Excellent, thank you!

…-Simulacrum Update backtrace submodule This pulls in a few notable changes (see the [complete list](rust-lang/backtrace-rs@99faef8...e9da96e) for details): - rust-lang/backtrace-rs#508 - rust-lang/backtrace-rs#566 - rust-lang/backtrace-rs#569 - Fixes rust-lang#116403 by no longer using `dbghelp.dll` during backtrace capture.

Update backtrace submodule This pulls in a few notable changes (see the [complete list](rust-lang/backtrace-rs@99faef8...e9da96e) for details): - rust-lang/backtrace-rs#508 - rust-lang/backtrace-rs#566 - rust-lang/backtrace-rs#569 - Fixes #116403 by no longer using `dbghelp.dll` during backtrace capture.

bzEq marked this pull request as draft January 12, 2023 09:06

bzEq force-pushed the add-aix-support branch from 78f1a49 to a04ee0c Compare May 4, 2023 03:00

bzEq marked this pull request as ready for review May 8, 2023 04:14

bzEq force-pushed the add-aix-support branch from cd6a1c7 to a9b86f5 Compare May 24, 2023 08:31

workingjubilee closed this Jun 22, 2023

workingjubilee reopened this Jun 22, 2023

bzEq force-pushed the add-aix-support branch from a9b86f5 to 26d171e Compare June 28, 2023 07:48

workingjubilee closed this Jun 28, 2023

workingjubilee reopened this Jun 28, 2023

workingjubilee reviewed Jun 29, 2023

View reviewed changes

bzEq force-pushed the add-aix-support branch from b1d22fb to c176648 Compare July 13, 2023 02:42

bzEq requested review from workingjubilee and bjorn3 July 13, 2023 03:08

workingjubilee reviewed Aug 18, 2023

View reviewed changes

Add AIX support

ecb214b

- Use `loadquery` to get loaded libraries - Use object crate to parse XCOFF and get image information

bzEq force-pushed the add-aix-support branch from 903ea3a to ecb214b Compare August 23, 2023 08:27

bzEq requested a review from workingjubilee September 5, 2023 02:46

workingjubilee approved these changes Sep 9, 2023

View reviewed changes

workingjubilee merged commit b471e5f into rust-lang:master Sep 9, 2023
35 checks passed

wesleywiser mentioned this pull request Oct 23, 2023

Update backtrace submodule rust-lang/rust#117089

Merged

workingjubilee mentioned this pull request Aug 2, 2024

Apply some of the cargo clippy suggestions #649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AIX support in gimli symbolizer #508

Add AIX support in gimli symbolizer #508

bzEq commented Jan 12, 2023

ecnelises commented Apr 27, 2023

workingjubilee commented Jun 22, 2023

bzEq commented Jun 28, 2023

workingjubilee left a comment

workingjubilee Jun 29, 2023 •

edited

Loading

bzEq Jul 5, 2023

workingjubilee Jul 5, 2023 •

edited

Loading

workingjubilee commented Jun 29, 2023 •

edited

Loading

bzEq commented Jul 5, 2023

workingjubilee commented Jul 5, 2023

bzEq commented Jul 25, 2023

workingjubilee left a comment

workingjubilee Aug 18, 2023

workingjubilee Aug 18, 2023

workingjubilee Aug 18, 2023

workingjubilee commented Aug 18, 2023

bzEq commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023

bzEq commented Sep 8, 2023

workingjubilee commented Sep 9, 2023

-                if Error::last_os_error().raw_os_error().unwrap() != libc::ENOMEM {
-                    return Vec::new();
-                }
-                buffer.resize(buffer.len() * 2, mem::zeroed::<libc::ld_info>());
+                 match Error::last_os_error().raw_os_error() {
+                     Some(libc::ENOMEM) => buffer.resize(buffer.len() * 2, mem::zeroed::<libc::ld_info>()),
+                     Some(libc::EINVAL | libc::EFAULT) => /* something? */,
+                     _ => unreachable!(),
+                 }

Add AIX support in gimli symbolizer #508

Add AIX support in gimli symbolizer #508

Conversation

bzEq commented Jan 12, 2023

ecnelises commented Apr 27, 2023

workingjubilee commented Jun 22, 2023

bzEq commented Jun 28, 2023

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

bzEq Jul 5, 2023

Choose a reason for hiding this comment

workingjubilee Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

workingjubilee commented Jun 29, 2023 • edited Loading

bzEq commented Jul 5, 2023

workingjubilee commented Jul 5, 2023

bzEq commented Jul 25, 2023

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Aug 18, 2023

Choose a reason for hiding this comment

workingjubilee Aug 18, 2023

Choose a reason for hiding this comment

workingjubilee Aug 18, 2023

Choose a reason for hiding this comment

workingjubilee commented Aug 18, 2023

bzEq commented Aug 23, 2023 • edited Loading

github-actions bot commented Aug 23, 2023

bzEq commented Sep 8, 2023

workingjubilee commented Sep 9, 2023

workingjubilee Jun 29, 2023 •

edited

Loading

workingjubilee Jul 5, 2023 •

edited

Loading

workingjubilee commented Jun 29, 2023 •

edited

Loading

bzEq commented Aug 23, 2023 •

edited

Loading