[PATCH] D11969: Avoid using of DataLayout::getPointerSize() without address space argument in DWARF info generation

[Eric Christopher via llvm-commits]

On Thu, Aug 13, 2015 at 6:59 PM Mekhanoshin, Stanislav <
Stanislav.Mekhanoshin at amd.com> wrote:

> > I think if you want to have disparate pointer sizes here you're going to
> need to change the DataLayout to encode a pointer size per address space or
> look at the places where we call getPointerSize() and actually somehow get
> the real size of the variable via something else. This might end up
> necessarily going back to DataLayout and a pointer size per address space.
>
>
>
> Yes, the only issue is source variable is not available at these places,
> so needs to be passed. The current implementation always query for a
> pointer in address space zero, which is even more incorrect.
>
>
>

I pretty much agree with Mehdi's response up thread from here on this
subject. At the least I don't have anything to add.

-eric


> Stas
>
>
>
> *From:* Eric Christopher [mailto:echristo at gmail.com]
> *Sent:* Thursday, August 13, 2015 6:55 PM
> *To:* Mekhanoshin, Stanislav <Stanislav.Mekhanoshin at amd.com>; Mehdi Amini
> <mehdi.amini at apple.com>
> *Cc:* reviews+D11969+public+c218119e32184906 at reviews.llvm.org; Arsenault,
> Matthew <Matthew.Arsenault at amd.com>; llvm-commits at lists.llvm.org
>
>
> *Subject:* Re: [PATCH] D11969: Avoid using of
> DataLayout::getPointerSize() without address space argument in DWARF info
> generation
>
>
>
>
>
> On Thu, Aug 13, 2015 at 6:46 PM Mekhanoshin, Stanislav <
> Stanislav.Mekhanoshin at amd.com> wrote:
>
> > If it is to replace this with a getPointerSize() that has no clear
> attached semantic, I don’t see this as a progress.
>
> It looks like the question now is in the correct name of the function. It
> really looks suspicious to call it CodePointerSize in this case. Largest
> seems to be better because that is what actually happens there - pointer
> size is used in different contexts, but largest possible size works for all
> of them.
>
>
>
> The trick is that pointer size is going to be a dwarf type as pointer to
> <thing> and then used all over the place. It's a clever, but inaccurate
> workaround that "largest wins" here. It would stop you from correctly using
> something like say lldb to reconstruct function calls etc.
>
>
>
> I think if you want to have disparate pointer sizes here you're going to
> need to change the DataLayout to encode a pointer size per address space or
> look at the places where we call getPointerSize() and actually somehow get
> the real size of the variable via something else. This might end up
> necessarily going back to DataLayout and a pointer size per address space.
>
>
>
> -eric
>
>
>
> Stas
>
> -----Original Message-----
> From: Mehdi Amini [mailto:mehdi.amini at apple.com]
> Sent: Thursday, August 13, 2015 6:32 PM
> To: Mekhanoshin, Stanislav <Stanislav.Mekhanoshin at amd.com>
> Cc: reviews+D11969+public+c218119e32184906 at reviews.llvm.org; Arsenault,
> Matthew <Matthew.Arsenault at amd.com>; echristo at gmail.com;
> llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D11969: Avoid using of DataLayout::getPointerSize()
> without address space argument in DWARF info generation
>
>
> > On Aug 13, 2015, at 6:26 PM, Mekhanoshin, Stanislav <
> Stanislav.Mekhanoshin at amd.com> wrote:
> >
> > Yes, I agree. A better longer term solution will require to dig in into
> each use of it.
> > At the very least I think we need to remove
> TargetMachine.getPointerSize() and AsmPrinter.getPointerSize() not to
> increase a number of such functions without an argument.
>
> If it is to replace this with a getPointerSize() that has no clear
> attached semantic, I don’t see this as a progress.
>
> —
> Mehdi
>
>
> > I'm testing this conservative patch now.
> >
> > Stas
> >
> > -----Original Message-----
> > From: Mehdi Amini [mailto:mehdi.amini at apple.com]
> > Sent: Thursday, August 13, 2015 6:21 PM
> > To: reviews+D11969+public+c218119e32184906 at reviews.llvm.org
> > Cc: Mekhanoshin, Stanislav <Stanislav.Mekhanoshin at amd.com>; Arsenault,
> Matthew <Matthew.Arsenault at amd.com>; echristo at gmail.com;
> llvm-commits at lists.llvm.org
> > Subject: Re: [PATCH] D11969: Avoid using of DataLayout::getPointerSize()
> without address space argument in DWARF info generation
> >
> >
> >> On Aug 13, 2015, at 5:35 PM, Stanislav Mekhanoshin <
> stanislav.mekhanoshin at amd.com> wrote:
> >>
> >> rampitec added a comment.
> >>
> >>> In http://reviews.llvm.org/D11969#223472, @joker.eph wrote:
> >>
> >>>
> >>
> >>>> - The DataLayout does not defined anything about which address space
> the code resides in
> >>
> >>>
> >>
> >>
> >> I have checked with our engineer doing debug info, and it he confirmed
> these uses are not related to code addresses,
> >
> > Not nice, my local debug expert had a different interpretation :(
> >
> > It seems that every single use of this getPointerSize() will need to be
> studied to see what it really means and what is the “right” answer.
> >
> > Right now MCDwarf.cpp is the only user of MCAsmInfo::getPointerSize() as
> far I can see.
> >
> >> but generally to variable addresses. In fact in our case all code
> addresses are redirected, so we should not even hit it. Then a bigger type
> holding pointers is an inefficiency, but not a bug.
> >>
> >> I.e. it looks like the name "CodePointerSize" is not correct, a
> "PointerSize" makes more sense. Maybe a "getLargestPointerSize()" and a
> "LargestPointerSize" would be better here.
> >> It can even be calculated from the DataLayout itself.
> >
> > It is not clear to me that it is the *right* answer to provide a
> “getLargestPointerSize()”, even though it seems that would be enough for
> your use case
> >
> > —
> > Mehdi
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150814/b2f16476/attachment.html>