Why is Dwarf::borrow deprecated?

[danielocfb]

See title.

Introduced by e6e90c9, which states:

Also replace Dwarf::borrow with DwarfSections::borrow for consistency.

I looked for it, but couldn't find said consistency. Now we have borrow on DwarfSections and...nothing for Dwarf. What if Dwarf has a sup set and I want to borrow that? Seems now I am forced to DwarfSections::borrow_with_sup again, which allocates a new Arc with a sup which seems consistently worse than what was possible beforehand, right? Can we please consider reverting the deprecation?

[philipc]

I looked for it, but couldn't find said consistency

For consistency with how DwarfPackage needs to borrow from DwarfPackageSections, which is the primary addition in that commit. It also means users no longer need to create a Dwarf from fields, so adding more fields to Dwarf won't break them.

Seems now I am forced to DwarfSections::borrow_with_sup again, which allocates a new Arc with a sup which seems consistently worse than what was possible beforehand, right?

That was the intent. Dwarf::borrow also allocate a new Arc. How is that consistently worse?

Can we please consider reverting the deprecation?

Sure, but I'll need to understand the reason why it is needed.

[danielocfb]

Seems now I am forced to DwarfSections::borrow_with_sup again, which allocates a new Arc with a sup which seems consistently worse than what was possible beforehand, right?

That was the intent. Dwarf::borrow also allocate a new Arc. How is that consistently worse?

Oh lol. I skimmed the code and read it as doing a cheap Arc::clone.

In any case, it's worse because users are forced to do the same borrow_with_sup dance twice.

Can we please consider reverting the deprecation?

Sure, but I'll need to understand the reason why it is needed.

It's necessary for when a second Dwarf instance is needed because the first one has already been consumed, for example.

[philipc]

I still don't understand. Can you demonstrate with some code?

For example, here's what I think the two alternatives are, using code from the documentation examples:

let dwarf = owned_dwarf.borrow(|section| {
    gimli::EndianSlice::new(&section, gimli::LittleEndian)
});

let dwarf = dwarf_sections.borrow_with_sup(&dwarf_sup_sections, |section| {
    gimli::EndianSlice::new(&section, gimli::LittleEndian)
});

The only difference I can see is an extra argument, which seems like a trivial difference to me.

[philipc]

For an argument in favor of DwarfSections::borrow_with_sup, it lets you store the two DwarfSections in the same place, so it actually avoids an extra Arc.

[danielocfb]

Can you demonstrate with some code?

Here is an example:

    let sections = gimli::DwarfSections::load(/* .. */).unwrap();
    let dwarf = if let Some(...) = /* maybe-some-sup-object */ {
        let sup_secs = gimli::DwarfSections::load(/* load sections from sup object */).unwrap();
        sections.borrow_with_sup(&sup_secs, |b| *b)
    } else {
        sections.borrow(|b| *b)
    };

    let ctx = addr2line::Context::from_dwarf(dwarf).unwrap();

    // Down here we may need a `Dwarf` again. So now we need to keep the `sup_secs` around,
    // recreate everything again with the conditional `sections.borrow_with_sup` dance, instead
    // of just being able to pass `dwarf.borrow(|b| *b)` to `addr2line::Context::from_dwarf` and then
    // still have the "original".

[philipc]

Ok. That seems like a design mistake with borrow_with_sup. If we changed borrow_with_sup to take an Option instead, then you could do:

    let sections = gimli::DwarfSections::load(/* .. */).unwrap();
    let sup_secs = if let Some(...) = /* maybe-some-sup-object */ {
        Some(gimli::DwarfSections::load(/* load sections from sup object */).unwrap())
    } else {
        None
    };

    let dwarf = sections.borrow_with_sup(sup_secs.as_ref(), |b| *b)
    let ctx = addr2line::Context::from_dwarf(dwarf).unwrap();

    // Down here we may need a `Dwarf` again.
    let dwarf = sections.borrow_with_sup(sup_secs.as_ref(), |b| *b);

[philipc]

A better solution is gimli-rs/addr2line#327, which allows you to use the same Dwarf in both places.

[danielocfb]

Yep, either should work for the use case at hand. Thanks!