Re: [clang] d9b9621 - Reland D73534: [DebugInfo] Enable the debug entry values feature by default

[Djordje via cfe-commits]

(+ CCing some debug info folks as well)

Hi David,

OK, I agree, my apologize.

The patch enables the whole feature by default and removes the experimental 
option that has been used during the implementation.
We tested the code (and the assertions regarding the feature itself) by using 
the experimental option on some large projects, but since the feature became 
that huge (and new patches regarding the feature are coming every day -- which 
is nice), it is impossible to test everything.

The Debug Entry Values implementation grows these days, and new functionalities 
comes in, but it has all been tested with the experimental option, so 
committing this particular patch imposes all the cases developers oversighted 
during the implementation.

My impression is that, before adding new functionalities to the feature, we 
should make sure the D73534 is stable and gets stuck. We should have done it 
before, but I think that is the lesson learned here.

In general, experimental options are very good up to the some point, but if the 
feature being tested grows a lot, a "breakpoint" should be set up and take care 
the experimental option is removed and the code is well tested on the real 
cases out there, and then moving to the next stage of the implementation.

Besides this, CISCO is using the feature ON by default for some time, and we 
have good experience there. This really improves the debugging user experience 
while debugging the "optimized" code.

Best regards,
Djordje

On 22.3.20. 16:30, David Blaikie wrote:
> Also, this patch was reverted and recommitted several times (more than I 
> think would be ideal) - please include more details in the commit message 
> about what went wrong, what was fixed, what testing was missed in the first 
> place and done in subsequent precommit validation (& is there anything more 
> broadly we could learn from this patches story to avoid this kind of 
> revert/recommit repetition in the future?)
>
> On Sat, Mar 21, 2020 at 8:13 PM David Blaikie  > wrote:
>
> Please include the "Differential Revision" line so that Phab picks up 
> commits like this and ties them into the review (& also makes it conveniently 
> clickable to jump from the commit mail to the review)
>
> On Thu, Mar 19, 2020 at 5:58 AM Djordje Todorovic via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
>
>
> Author: Djordje Todorovic
> Date: 2020-03-19T13:57:30+01:00
> New Revision: d9b962100942c71a4c26debaa716f7ab0c4ea8a1
>
> URL: 
> https://github.com/llvm/llvm-project/commit/d9b962100942c71a4c26debaa716f7ab0c4ea8a1
> DIFF: 
> https://github.com/llvm/llvm-project/commit/d9b962100942c71a4c26debaa716f7ab0c4ea8a1.diff
>
> LOG: Reland D73534: [DebugInfo] Enable the debug entry values feature 
> by default
>
> The issue that was causing the build failures was fixed with the 
> D76164.
>
> Added:
>     llvm/test/DebugInfo/X86/no-entry-values-with-O0.ll
>
> Modified:
>     clang/include/clang/Basic/CodeGenOptions.def
>     clang/include/clang/Driver/CC1Options.td
>     clang/lib/CodeGen/BackendUtil.cpp
>     clang/lib/CodeGen/CGDebugInfo.cpp
>     clang/lib/Frontend/CompilerInvocation.cpp
>     clang/test/CodeGen/debug-info-extern-call.c
>     clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp
>     lldb/packages/Python/lldbsuite/test/decorators.py
>     
> lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/Makefile
>     llvm/include/llvm/Target/TargetMachine.h
>     llvm/include/llvm/Target/TargetOptions.h
>     llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>     llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
>     llvm/lib/CodeGen/CommandFlags.cpp
>     llvm/lib/CodeGen/LiveDebugValues.cpp
>     llvm/lib/CodeGen/TargetOptionsImpl.cpp
>     llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
>     llvm/lib/Target/ARM/ARMTargetMachine.cpp
>     llvm/lib/Target/X86/X86TargetMachine.cpp
>     llvm/test/CodeGen/MIR/Hexagon/bundled-call-site-info.mir
>     llvm/test/CodeGen/MIR/X86/call-site-info-error4.mir
>     llvm/test/CodeGen/X86/call-site-info-output.ll
>     llvm/test/DebugInfo/AArch64/dbgcall-site-float-entry-value.ll
>     llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-orr-moves.mir
>     llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir
>     llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir
>     llvm/test/DebugInfo/MIR/ARM/dbgcall-site-propagated-value.mir
>     
> llvm/test/DebugInfo/MIR/Hexagon/dbgcall-site-instr-before-bundled-call.mir
>     
> llvm/test/DebugInfo/MIR/Hexagon/live-debug-values-bundled-entry-values.mir
>     llvm/test/DebugInfo/MIR/SystemZ/call-site-lzer.mir
>     llvm/test/DebugInfo/MIR/X86/DW_OP_entr

Re: [clang] d9b9621 - Reland D73534: [DebugInfo] Enable the debug entry values feature by default

[Djordje via cfe-commits]

Hi David,

Thanks for the heads-up!

On 23.3.20. 11:02, David Stenberg wrote:
> Hi!
>
> On Mon, 2020-03-23 at 08:56 +0100, Djordje wrote:
>> (+ CCing some debug info folks as well)
>>
>> Hi David,
>>
>> OK, I agree, my apologize.
>>
>> The patch enables the whole feature by default and removes the experimental
>> option that has been used during the implementation.
>> We tested the code (and the assertions regarding the feature itself) by using
>> the experimental option on some large projects, but since the feature became
>> that huge (and new patches regarding the feature are coming every day -- 
>> which
>> is nice), it is impossible to test everything.
> When the issue related to D75036 was reported I probably should have reverted
> that commit directly when starting to troubleshoot the issue, so that we 
> didn't
> have to end up reverting D73534.
>
> Sorry about that!
>
>> The Debug Entry Values implementation grows these days, and new 
>> functionalities
>> comes in, but it has all been tested with the experimental option, so
>> committing this particular patch imposes all the cases developers oversighted
>> during the implementation.
>>
>> My impression is that, before adding new functionalities to the feature, we
>> should make sure the D73534 is stable and gets stuck. We should have done it
>> before, but I think that is the lesson learned here.
> That sounds reasonable I think. For how long should we do that? A week? Two
> weeks?
I am not sure, but I guess two weeks are fine.
>> In general, experimental options are very good up to the some point, but if 
>> the
>> feature being tested grows a lot, a "breakpoint" should be set up and take 
>> care
>> the experimental option is removed and the code is well tested on the real
>> cases out there, and then moving to the next stage of the implementation.
>>
>> Besides this, CISCO is using the feature ON by default for some time, and we
>> have good experience there. This really improves the debugging user 
>> experience
>> while debugging the "optimized" code.
>>
>> Best regards,
>> Djordje
> Best regards,
> David S

Thanks,
Djordje

>> On 22.3.20. 16:30, David Blaikie wrote:
>>> Also, this patch was reverted and recommitted several times (more than I
>>> think would be ideal) - please include more details in the commit message
>>> about what went wrong, what was fixed, what testing was missed in the first
>>> place and done in subsequent precommit validation (& is there anything more
>>> broadly we could learn from this patches story to avoid this kind of
>>> revert/recommit repetition in the future?)
>>>
>>> On Sat, Mar 21, 2020 at 8:13 PM David Blaikie >> dblai...@gmail.com>> wrote:
>>>
>>> Please include the "Differential Revision" line so that Phab picks up
>>> commits like this and ties them into the review (& also makes it 
>>> conveniently
>>> clickable to jump from the commit mail to the review)
>>>
>>> On Thu, Mar 19, 2020 at 5:58 AM Djordje Todorovic via cfe-commits <
>>> cfe-commits@lists.llvm.org > wrote:
>>>
>>>
>>> Author: Djordje Todorovic
>>> Date: 2020-03-19T13:57:30+01:00
>>> New Revision: d9b962100942c71a4c26debaa716f7ab0c4ea8a1
>>>
>>> URL: 
>>> https://protect2.fireeye.com/v1/url?k=77e8c227-2b3ccf55-77e882bc-86859b2931b3-a6817f347a752f02&q=1&e=88d8925e-f175-4c81-9485-741ddc24573f&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fcommit%2Fd9b962100942c71a4c26debaa716f7ab0c4ea8a1
>>> DIFF: 
>>> https://protect2.fireeye.com/v1/url?k=3914be57-65c0b325-3914fecc-86859b2931b3-b52b8ad09d192520&q=1&e=88d8925e-f175-4c81-9485-741ddc24573f&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fcommit%2Fd9b962100942c71a4c26debaa716f7ab0c4ea8a1.diff
>>>
>>> LOG: Reland D73534: [DebugInfo] Enable the debug entry values 
>>> feature
>>> by default
>>>
>>> The issue that was causing the build failures was fixed with the
>>> D76164.
>>>
>>> Added:
>>> llvm/test/DebugInfo/X86/no-entry-values-with-O0.ll
>>>
>>> Modified:
>>> clang/include/clang/Basic/CodeGenOptions.def
>>> clang/include/clang/Driver/CC1Options.td
>>> clang/lib/CodeGen/BackendUtil.cpp
>>> clang/lib/CodeGen/CGDebugInfo.cpp
>>> clang/lib/Frontend/CompilerInvocation.cpp
>>> clang/test/CodeGen/debug-info-extern-call.c
>>> clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp
>>> lldb/packages/Python/lldbsuite/test/decorators.py
>>>
>>> lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/Make
>>> file
>>> llvm/include/llvm/Target/TargetMachine.h
>>> llvm/include/llvm/Target/TargetOptions.h
>>> llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>> llvm/lib/CodeGen/CommandFlags.cpp
>>> llvm/lib/CodeGen/LiveDebugValues.cpp
>>>

Re: [clang] d9b9621 - Reland D73534: [DebugInfo] Enable the debug entry values feature by default

[Djordje via cfe-commits]

Hi David,

On 23.3.20. 18:51, David Blaikie wrote:
> On Mon, Mar 23, 2020 at 12:56 AM Djordje  > wrote:
>
> (+ CCing some debug info folks as well)
>
> Hi David,
>
> OK, I agree, my apologize.
>
> The patch enables the whole feature by default and removes the 
> experimental option that has been used during the implementation.
> We tested the code (and the assertions regarding the feature itself) by 
> using the experimental option on some large projects, but since the feature 
> became that huge (and new patches regarding the feature are coming every day 
> -- which is nice), it is impossible to test everything.
>
>
> Certainly not possible to test everything - and sometimes testing on the 
> buildbots is the best we have, though in that  case (when you know there's 
> likely some "testing in production" to do) it's probably best to commit it 
> during an off-peak time if possible (not always possible, not asking you to 
> work weekends, etc) and plan to revert it once you get the buildbot feedback 
> you need to make progress, etc.
>
> The other thing I'm interested in/suggesting is that when you commit 
> something and it fails on buildbots, it gets reverted - especially with 
> something big like this, I think it's important/worthwhile to look at what 
> testing wasn't done pre-commit that found failures post-commit, and not just 
> addressing the specific failure the buildbots found, but rerunning the 
> broader set of testing (if it's possible for you to do so - you might not 
> haev the available hardware, etc - but if it's about building with 
> sanitizers, building with different targets enabled, etc) as well, as 
> generalized as much as possible, to increase the chances the next time around.
>
> That's what I expect/hope to see when something is recommitted "This was 
> reverted in XXX due to  which was addressed in  and 
> validated with ".
>  
OK, I understand, thanks a lot for the feedback! :)
>
>
> The Debug Entry Values implementation grows these days, and new 
> functionalities comes in, but it has all been tested with the experimental 
> option, so committing this particular patch imposes all the cases developers 
> oversighted during the implementation.
>
> My impression is that, before adding new functionalities to the feature, 
> we should make sure the D73534 is stable and gets stuck. We should have done 
> it before, but I think that is the lesson learned here.
>
> In general, experimental options are very good up to the some point, but 
> if the feature being tested grows a lot, a "breakpoint" should be set up and 
> take care the experimental option is removed and the code is well tested on 
> the real cases out there, and then moving to the next stage of the 
> implementation.
>
>
> Yes, generally the idea with an experimental feature like this one where the 
> cost/benefit (size increase compared to debug info usability improvements) is 
> in question, etc would be, imho, to stabilize the feature representing the 
> best (or at least a "good enough") cost/benefit, etc, and then make it 
> non-experimental - if it's still a moving target/with changes being made, 
> maybe it's still experimental?
Speaking of this feature, we agreed it is ready to be ON by default, since the 
debug info statistics was shown, time/size costs, benefits to end users etc.. 
But, we have also continued to adding new code (besides fixes the bugs imposed 
by this patch), which is great, but it imposed new failures and new 
reverts/recommits.
>  
>
> Besides this, CISCO is using the feature ON by default for some time, and 
> we have good experience there. This really improves the debugging user 
> experience while debugging the "optimized" code.
>
> Best regards,
> Djordje
>
> On 22.3.20. 16:30, David Blaikie wrote:
> > Also, this patch was reverted and recommitted several times (more than 
> I think would be ideal) - please include more details in the commit message 
> about what went wrong, what was fixed, what testing was missed in the first 
> place and done in subsequent precommit validation (& is there anything more 
> broadly we could learn from this patches story to avoid this kind of 
> revert/recommit repetition in the future?)
> >
> > On Sat, Mar 21, 2020 at 8:13 PM David Blaikie    >> wrote:
> >
> >     Please include the "Differential Revision" line so that Phab picks 
> up commits like this and ties them into the review (& also makes it 
> conveniently clickable to jump from the commit mail to the review)
> >
> >     On Thu, Mar 19, 2020 at 5:58 AM Djordje Todorovic via cfe-commits 
> mailto:cfe-commits@lists.llvm.org> 
> >> 
> wrote:
> >
> >
> >         Author: Djordje Todorovic
> >         Date: 2020-03-19T13:57:3