fix: Use correct symbol priority when primary definition is dynamic#1601
Conversation
| self.parsed.object.is_dynamic() | ||
| } | ||
|
|
||
| pub(crate) fn symbol_strength(&self, symbol_id: crate::symbol_db::SymbolId) -> SymbolStrength { |
There was a problem hiding this comment.
It looks like this function is more or less a copy of ObjectFile::symbol_strength. Can you deduplicate?
| let mut max_common: Option<(u64, SymbolId)> = None; | ||
| let mut first_weak: Option<SymbolId> = None; | ||
|
|
||
| for &alt in alternatives { | ||
| let file_id = symbol_db.file_id_for_symbol(alt); | ||
| let file = symbol_db.file(file_id); | ||
| if file.is_dynamic() { | ||
| continue; | ||
| } | ||
| let strength = file.symbol_strength(alt); | ||
| match strength { | ||
| SymbolStrength::Strong => { | ||
| return Some(alt); | ||
| } | ||
| SymbolStrength::Weak | SymbolStrength::GnuUnique => { | ||
| if first_weak.is_none() { | ||
| first_weak = Some(alt); | ||
| } | ||
| } | ||
| SymbolStrength::Common(size) => { | ||
| if let Some((prev_size, _)) = max_common { | ||
| if size > prev_size { | ||
| max_common = Some((size, alt)); | ||
| } | ||
| } else { | ||
| max_common = Some((size, alt)); | ||
| } | ||
| } | ||
| SymbolStrength::Undefined => {} | ||
| } | ||
| } | ||
|
|
||
| if let Some((_, id)) = max_common { | ||
| return Some(id); | ||
| } | ||
|
|
||
| first_weak |
There was a problem hiding this comment.
The logic here seems to be more or less the same as select_symbol. Can they share code?
There was a problem hiding this comment.
I suspect this test passes even without your change. Can you adapt the test so that tests what you changed? You'll need to look at where get_non_dynamic is called and figure out when that code path is exercised. That will hopefully also allow you to make the commit message more specific as to what's being fixed.
There was a problem hiding this comment.
That makes sense now. However, I just tried reverting your non test changes and the test still passes. Does it fail for you?
There was a problem hiding this comment.
It passes for me as well
There was a problem hiding this comment.
Any idea why it passes without your fix?
There was a problem hiding this comment.
The test passes without the fix because of how definition() works with two-level indirection. So the test can't distinguish between the old and new get_non_dynamic because select_symbol + definition() always resolves the correct winner regardless of which non-dynamic alternative get_non_dynamic picks.
Let me update it.
There was a problem hiding this comment.
Thanks. I can confirm that the test now fails for me without your fix.
|
All 3 review comments have been addressed. |
fc38d15 to
58a51b8
Compare
There was a problem hiding this comment.
Thanks. I can confirm that the test now fails for me without your fix.
There was a problem hiding this comment.
I think the names of the two secondary files now don't make so much sense since they each contain one weak and one strong symbol.
There was a problem hiding this comment.
Do you have a suggestion for better names?
There was a problem hiding this comment.
I'm not sure what you're asking
There was a problem hiding this comment.
Sorry, I got a bit lost.
There was a problem hiding this comment.
The two files have been renamed.
This pull request improves the symbol resolution logic to correctly prioritize symbol bindings (strong, common, weak, GNU unique) when multiple object files define the same symbol. It also adds new integration tests to verify the correct behavior.