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

Wed Jul 3 08:50:25 PDT 2019

dblaikie added a comment.

In D64006#1567868 <https://reviews.llvm.org/D64006#1567868>, @ikudrin wrote:

> In D64006#1566831 <https://reviews.llvm.org/D64006#1566831>, @aprantl wrote:
>
> > ...in places where it likely won't be needed any time soon, such as 64-bit DWARF parsing support.
>
>
> In fact, I am going to implement 64-bit DWARF support. I started from `DataExtractor` because changing it seems inevitable for that anyway.
>
> In D64006#1567394 <https://reviews.llvm.org/D64006#1567394>, @dblaikie wrote:
>
> > 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?
>
>
> In the beginning, I also thought that having only one possible type for offsets, `uint64_t`, is preferable. But thinking of that for some time, I cannot find any strong reason not to keep both variants, apart from some kind of aesthetic sense, which is debatable. I really do not see any new harm which may result from having overloads for both 32- and 64-bit offsets. The only issue I can guess is a potential risk of wrapping a 32-bit offset value, but the current implementation is also affected by that, so nothing new is here.

I think the main one is code duplication - write once/fix once/etc is valuable.

> Moreover, this is just a utility class. It should provide its users with functionality, not force them to satisfy its whims, especially if that complicates things for them without any noticeable value; I mean, storing 32-bit offsets and creating a temporary 64-bit variable just to call the utility class does not seem aesthetic, too. It is the callers who know how big their data is and which data type for offsets reflects their needs better.

I think if we knew then what we know now we'd have built it with 64 bit offsets & it wouldn't've been that much imposition to clients that can use 32 bit offsets.

Did you come across cases where you would need to insert new 64 bit temporaries? My understanding would be that anything storing a lot of offsets in a data structure would be storing fixed offsets that were not intended to be mutated (eg: the DIEs in a DIE tree might contain an offset to the start of the attributes in that DIE) - so you wouldn't want to pass the address of that offset directly to DataExtractor (because it would mutate it, then the DIE in the DIE tree would have an offset that no longer points to the start of the attributes - but somewhere in the middle/end - making it unusable from then on) - so such code would likely copy the offset into a local intended for mutation. The difference now is that local would be 64 bit. That doesn't seem like an imposition to me.

> Anyway, as I said, my main intent is to add support for 64-bit DWARF. While there are other users, `DataExtractor` is mainly used in the `DebugInfo/DWARF` library and I expect to change it to use 64-bit offsets while implementing that support. Hopefully, that will be done with a bunch of relatively small patches. After that, we can decide what to do with the remaining callers. Unfortunately, I cannot promise that this transition may be done in several weeks. It seems that two to four months is a more realistic estimation.

Fair enough - though would it be possible to prioritize finishing the DataExtractor migration (or demonstrating it is not desirable) before necessarily fleshing out the rest of the DWARF 64 support? I'd be concerned it might be left lingering otherwise.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64006