Support DW_FORM_addrx* in low_pc and high_pc #541
Conversation
| @@ -629,6 +633,11 @@ impl<R: Reader> Unit<R> { | |||
| )?), | |||
| None => None, | |||
| }; | |||
| if let Some(low_pc_attr) = low_pc_attr { | |||
| if let Some(addr) = dwarf.attr_address(&unit, low_pc_attr)? { | |||
There was a problem hiding this comment.
Note: to be consistent, I followed example of unit.{name,comp_dir,line_program} above where invalid attribute form is silently ignored, but I'm not sure why it's done that way...
Wouldn't it be better to return explicit UnsupportedAttributeForm error in all those cases?
There was a problem hiding this comment.
This code originated from the dwarfdump example (a5c7ac0) and the strategy there was to be able to dump as much as we can, even if it is potentially invalid.
I agree that ideally we would return an error, and this would help expose bugs more quickly. On the other hand not all consumers need everything that this function parses. For example, #515 was fine with ignoring errors instead of fixing the real problem (missing supplementary object file).
I don't know the best way to handle this. Do you have any ideas?
There was a problem hiding this comment.
Hmm, not sure either. I guess personally I would turn those fields into private OnceCells, and expose fallible getters as functions for them instead - that way, user could both handle an error, and not sacrifice any performance because variable would be cached once retrieved.
However, this would be a breaking API change, so probably not acceptable?
There was a problem hiding this comment.
Not sure I properly understand what you mean. Using OnceCell, retrieving the variable would need to iterate the attributes again in order to initialize the OnceCell?
Breaking changes are fine if we are sure it is the correct thing to do (but for this case I'm not sure). We still don't support all of DWARF 5, and that is likely to require more breaking changes.
There was a problem hiding this comment.
retrieving the variable would need to iterate the attributes again in order to initialize the OnceCell
Yeah, API-wise it would be equivalent of DebuggingInformationEntry::attr(name) which already does that, but here it would do it with caching. As you said yourself, many consumers are already fine with ignoring some fields or errors in them.
I think most consumers don't actually need to access all of the Unit properties, and emitters usually don't emit many properties on Unit node, so iteration won't have any noticeable performance impact.
There was a problem hiding this comment.
As another alternative, you could keep current iteration, but store Results in private fields, and clone them in such getters. But I really think that making field accessors lazy makes more sense for common scenarios, as it will reduce costs for parsing things user doesn't care about.
|
Other than the comment above, checked and this works as expected on my original DWARF example. |
| @@ -629,6 +633,11 @@ impl<R: Reader> Unit<R> { | |||
| )?), | |||
| None => None, | |||
| }; | |||
| if let Some(low_pc_attr) = low_pc_attr { | |||
| if let Some(addr) = dwarf.attr_address(&unit, low_pc_attr)? { | |||
There was a problem hiding this comment.
This code originated from the dwarfdump example (a5c7ac0) and the strategy there was to be able to dump as much as we can, even if it is potentially invalid.
I agree that ideally we would return an error, and this would help expose bugs more quickly. On the other hand not all consumers need everything that this function parses. For example, #515 was fine with ignoring errors instead of fixing the real problem (missing supplementary object file).
I don't know the best way to handle this. Do you have any ideas?
|
Could you please publish a patch release for this fix? For now I'm pointing to my Github fork with |
|
I can't because there's already breaking changes on master, and in any case I'd prefer to wait a bit to be sure you don't need more fixes. |
|
Ah okay. So far this is the only thing I ran into, I don't think I'll need anything else on Gimli side in the nearest future, but I guess makes sense to wait. |
Fixes #540.