[PATCH] D64006: [Support] Support 64-bit offsets in DataExtractor.

Tue Jul 2 15:36:21 PDT 2019

dblaikie added a comment.

In D64006#1566831 <https://reviews.llvm.org/D64006#1566831>, @aprantl wrote:

> Thank you for demonstrating this! I agree that the patch also introduces lots of opportunities for subtle bugs. It also increases the memory footprint in places where it likely won't be needed any time soon, such as 64-bit DWARF parsing support. This patch looks much more palatable to me now and I'd be okay with taking it if @dblaikie is okay with it. Is there danger in using the same function name or should we call the 64-bit variants something more explicit?

I don't feel /super/ strongly that it be done in one go. But it sounds like @ikudrin is suggesting maybe it's not their goal to migrate from 32 to 64 entirely, and that maybe it's OK to have both APIs? I really rather that not be the case. I think if there are negative size impacts for this migration to users of the DataExtractor API those can be worked around by certain clients continuing to store 32 bit offsets in data structures (but mostly these offsets aren't intended to be updated - so their addresses won't be passed directly to DataExtractor) and copying those into 64 bit values before using them as cursors into DataExtractor.

I'm OK with this patch direction - but only so long as there's a plan in weeks (probably not months, definitely not years) to migrate all callers over to 64 bit cursors.

(I could be convinced otherwise - but I think the bar would/should be pretty high to justify keeping both of these)

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64006/new/

https://reviews.llvm.org/D64006