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

Detect more ELF symbol kinds #498

Merged
merged 1 commit into from Jan 4, 2023
Merged

Detect more ELF symbol kinds #498

merged 1 commit into from Jan 4, 2023

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Jan 3, 2023

This makes 2 changes to SymbolKind detection:

  • STT_NOTYPE is detected as a label, since that's the default type given to a label in assembly code. This is also used for mapping symbols that are inserted by the assembler to indicate which addresses contain code or data.
  • STT_GNU_IFUNC is detected as a function symbol.

This makes 2 changes to `SymbolKind` detection:
- `STT_NOTYPE` is detected as a label, since that's the default type
given to a label in assembly code. This is also used for mapping symbols
that are inserted by the assembler to indicate which addresses contain
code or data.
- `STT_GNU_IFUNC` is detected as a function symbol.
@philipc philipc merged commit 4c05d01 into gimli-rs:master Jan 4, 2023
@mstange
Copy link
Contributor

mstange commented Jan 20, 2023

I like this change, but just for reference I'll note that this change being published in a patch release caused some minor breakage for me, see mstange/samply@8541504.

@philipc
Copy link
Contributor

philipc commented Jan 22, 2023

Do you think it's enough for this sort of change to be treated as a breaking change? It absolves this crate of blame, but doesn't do much to make it easier for consumers to notice that it might be a problem.

My feeling is that the consumer should never rely on something continuing to be in the Unknown category, since this is the category that is most likely to change in future.

See also #214 (comment), where the issue is that we need to provide other means for the consumer to handle it so that it doesn't matter if the category changes.

@mstange
Copy link
Contributor

mstange commented Jan 22, 2023

Do you think it's enough for this sort of change to be treated as a breaking change? It absolves this crate of blame, but doesn't do much to make it easier for consumers to notice that it might be a problem.

I don't have strong feelings either way. What was causing confusion for me was that I fixed a bug on my machine, and then found that the fix was not working when I tried it on a different machine. This was because I didn't have Cargo.lock in the repo, and the two machines were using different versions of object. So treating this as a breaking change would have avoided that particular source of confusion. (I have now made it so that Cargo.lock is in the repo.)

My feeling is that the consumer should never rely on something continuing to be in the Unknown category, since this is the category that is most likely to change in future.

I think that is a reasonable position.

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.

None yet

3 participants