[PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

Mon Jan 9 16:41:03 PST 2023

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D139379#4015624 <https://reviews.llvm.org/D139379#4015624>, @ayermolo wrote:

> In D139379#4015569 <https://reviews.llvm.org/D139379#4015569>, @dblaikie wrote:
>
>> In D139379#3972876 <https://reviews.llvm.org/D139379#3972876>, @ayermolo wrote:
>>
>>> In D139379#3972871 <https://reviews.llvm.org/D139379#3972871>, @dblaikie wrote:
>>>
>>>> Perhaps the change to use accessors could be removed, now that you've used it to find all the places that needed to be fixed up? (like just using it for cleanup/temporary purposes, without needing to commit that API change?)
>>>
>>> I am not sure what other projects are using it, that I missed, or not in llvm-trunk, but are based of it.
>>
>> It's awkward to convolute the API to ensure a breakage - I think it's best to leave it as-is, for the most part. I guess you needed the 32 bit accessors so existing code doesn't become UB because of truncation to 32 bit?
>>
>> Could the code keep the existing member names, provide the wrappers without turning the members into an array and needing named index constants, etc, at least? (though even then, seems like there's more to it than needed)
>
> Thanks for circling back to this during holidays. :)
> Right, that was my original thought pattern. I am fully open to the idea that this is not the right approach. :)

I don't know that there's enough out of tree usage to worry about forcing a break by changing the API like this - or to include the "get*32" functions.

Is this actually an undefined behavior issue (I don't think the implicit conversion would be any more broken than the explicit cast, at least - but can't find the specific wording about defined/undefined behavior off hand at the moment) , or only an attempt to identify places that could benefit from updates to support 64 bit lengths?

Eh, still seems weird, but guess it's not that much of a big deal - so let's go with it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139379