Skip to content

Conversation

@rjmansfield
Copy link
Contributor

No description provided.

Copy link
Collaborator

@EricRahm EricRahm left a comment

Choose a reason for hiding this comment

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

Overall lgtm, nice and straightforward. Thanks for including the test as well. Only small request is to add some details to the commit message and include the version of clang you used to generate the binary.

@rjmansfield
Copy link
Contributor Author

Overall lgtm, nice and straightforward. Thanks for including the test as well. Only small request is to add some details to the commit message and include the version of clang you used to generate the binary.

Sounds good. FWIW I tried to make this into a yaml test instead of using a prebuilt binary but I think I've found a obj2yaml/yaml2obj bug writing out the dsym. Since I wasn't sure how long it'll take to fix, and how long before the test could rely on updated yaml2obj, I opted for using a binary instead. However if you want, I can look at the yaml2obj bug more and then update the test with a comment to point to the llvm bug report (if needed)

@EricRahm
Copy link
Collaborator

EricRahm commented Nov 6, 2025

However if you want, I can look at the yaml2obj bug more and then update the test with a comment to point to the llvm bug report (if needed)

Having an upstream bug filed and referenced would be great.

@rjmansfield rjmansfield force-pushed the macho-inlines-support branch from 43609f3 to 03ba814 Compare November 7, 2025 19:02
@rjmansfield
Copy link
Contributor Author

However if you want, I can look at the yaml2obj bug more and then update the test with a comment to point to the llvm bug report (if needed)

Having an upstream bug filed and referenced would be great.

I was able to use generate a minimal YAML test and work around the upstream bug. llvm/llvm-project#166993 for reference.

Copy link
Collaborator

@EricRahm EricRahm left a comment

Choose a reason for hiding this comment

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

That test file is wild :) Given the complexity of the setup I think we're fine as-is, we can always come back and attempt to reduce it. Just a small request to more explicitly document the steps to generate it.

# Test that -d inlines works for Mach-O binaries
#
# The YAML below was generated from binaries compiled from
# tests/testdata/macho/test_inlines.c
Copy link
Collaborator

Choose a reason for hiding this comment

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

For completeness can we include the rough set of commands, I'm guessing vaguely:

# on macOS
# clang <some flags> tests/testdata/macho/test_inlines.c
# obj2yaml <some flags>
# manually change thing because bug xyz

@rjmansfield rjmansfield force-pushed the macho-inlines-support branch from 03ba814 to 015d015 Compare November 9, 2025 14:15
@rjmansfield
Copy link
Contributor Author

@EricRahm can you have another look at this when you have a chance?

@EricRahm
Copy link
Collaborator

EricRahm commented Dec 5, 2025

@EricRahm can you have another look at this when you have a chance?

Thanks for the reminder! I put some time on my schedule tomorrow. I'd like to also loop in @haberman since this has potential user facing changes.

@haberman
Copy link
Member

haberman commented Dec 5, 2025

This looks good to me. The test file is a little bigger than we generally like, but maybe that's necessary to account for the large amount of DWARF data generated for inlines.

@rjmansfield rjmansfield force-pushed the macho-inlines-support branch from 015d015 to cb04154 Compare December 5, 2025 15:09
@rjmansfield
Copy link
Contributor Author

This looks good to me. The test file is a little bigger than we generally like, but maybe that's necessary to account for the large amount of DWARF data generated for inlines.

OK, well I was able to trim it down a bit by ~27% by generating the yaml from the inline c test with -g1 without effecting the test behaviour.

Copy link
Collaborator

@EricRahm EricRahm left a comment

Choose a reason for hiding this comment

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

Thanks @rjmansfield, lgtm!

@EricRahm EricRahm merged commit f9a1183 into google:main Dec 6, 2025
9 checks passed
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.

3 participants