[cfe-dev] Track locations of individual loads in -Rpass remarks (Re: [polly] Add diagnostic remark for ReportVariantBasePtr)

[Tobias Grosser]

On 29/07/2014 08:22, Tobias Grosser wrote:
> On 28/07/2014 20:10, Eric Christopher wrote:
>> On Mon, Jul 28, 2014 at 10:38 AM, Diego Novillo <dnovillo at google.com>
>> wrote:
>>> On Thu, Jul 24, 2014 at 4:16 PM, Diego Novillo <dnovillo at google.com>
>>> wrote:
>>>> On Mon, Jul 21, 2014 at 9:36 AM, Tobias Grosser <tobias at grosser.es>
>>>> wrote:
>>>>
>>>>> Diego, you just recently run the -gcolumn-info test cases on your
>>>>> internal
>>>>> code base and on the gdb test suite. Would it be difficult for
>>>>> you to create similar information for the attached patch to get an
>>>>> idea of
>>>>> how much overhead this patch would cause?
>>>>
>>>> A full comparison build is now running. I should have results soon.
>>>> In the
>>>> meantime, I tried it on a target that generates a 30Mb shared object:
>>>>
>>>> Without your patch, I see these segment sizes:
>>>>
>>>>       text      data       bss     debug debug.dwo    symtab    strtab
>>>> other     total filename
>>>>    8505260    444768    308624  12140854         0   1226952   5631742
>>>> 1632669  29588116 file.so
>>>>
>>>>
>>>> With your patch:
>>>>       text      data       bss     debug debug.dwo    symtab    strtab
>>>> other     total filename
>>>>    8505260    444768    308624  12565490         0   1226952   5631742
>>>> 1632669  30012748 file.so
>>>>
>>>> That's a 3.5% growth in debug info size, for a total size increase
>>>> of ~1%.
>>>>
>>>> I'll send numbers over a full build as soon as I get them.
>>>
>>> I've got full numbers now. The extreme I chose was an outlier. I've
>>> done full builds over our code base. The increase on average debug
>>> sizes is 0.1%. The total file size increase is ~0%.
>>>
>>> I don't think your change will make a substantial difference in debug
>>> info sizes. At least, not in what I've observed.
>>>
>>
>> I'd have been surprised if it had, but thanks for running the numbers.
>
> Thanks Diego for testing. This is very much appreciated. It is good to
> run this on a large code basis to gain confidence in this change.
>
>> Tobi: Go ahead and commit it if you'd like. Please go ahead and watch
>> the bots to make sure there aren't any failures (they'll likely be
>> spurious, but we should watch out for them.
>
> Thanks. As I am not a debug system expert, it is good to confirm this is
> a reasonable approach.
>
>> One bit of review on the patch, looks like there are extra braces
>> since the bit after the conditional is a single line?
>
> Right, I also added a test case and updated three other test cases
> which checked full debug informations and where the column information
> of loads now changed.
>
> r214162

I was too quick and broke five gdb debug info tests:

http://lab.llvm.org:8011/builders/clang-x86_64-ubuntu-gdb-75/builds/16376

I looked in the consecutive reverse test. The expected execution is:

--------------------------------------------------------------------
 > break foo
Breakpoint 2 at 0x400594: file ./gdb.reverse/consecutive-reverse.c, line 27.

 > continue
Breakpoint 2, foo () at ./gdb.reverse/consecutive-reverse.c:27
27	  return a[0] + a[1] + a[2] + a[3] + a[4] + a[5] + a[6];

 > x /2i $pc
=> 0x400594 <foo+4>:    mov    0x601040,%eax
    0x40059b <foo+11>:   add    0x601044,%eax

 > step
Breakpoint 3, 0x000000000040059b in foo () at 
./gdb.reverse/consecutive-reverse.c:27
--------------------------------------------------------------------

However, after this change the last line becomes again:

Breakpoint 2, foo () at ./gdb.reverse/consecutive-reverse.c:27

I also tried with with gdb-7.6.1, which shows the same changed behavior. 
I need to investigate this more closely, but may not have the time this 
and next week. Thanks for your helo so far.

Cheers,
Tobias