Page MenuHomePhabricator

[DebugInfo] Enable the call site parameter feature by default
Needs ReviewPublic

Authored by alok on Mar 23 2021, 10:25 PM.

Details

Summary

This works with debug entry value feature, which works by default.
Without call site parameter feature, debug entry value feature is of no use.
With current changes, debuggers should be able to show optimized out parameters by default.

Diff Detail

Event Timeline

alok created this revision.Mar 23 2021, 10:25 PM
alok requested review of this revision.Mar 23 2021, 10:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 23 2021, 10:25 PM
alok updated this revision to Diff 332870.Mar 23 2021, 10:38 PM

Re-based after setting the parent patch.

djtodoro added inline comments.Mar 24 2021, 12:59 AM
clang/lib/Frontend/CompilerInvocation.cpp
1648

I am a bit confused now :)
Is flang using this clang's library?
I think we should be careful here, since I am not sure C/C++ -O0 code has any benefits of using this, but the DWARF size might be increased insanely.

llvm/test/DebugInfo/X86/callsitepar-fastisel.ll
12 ↗(On Diff #332870)

I don't think this should be tested this way. We should be testing Driver itself.

alok added inline comments.Apr 1 2021, 3:46 AM
clang/lib/Frontend/CompilerInvocation.cpp
1648

Classic flang calls same library. Below is the verbose output of "flang" command.
flang -g -O0 -v vla-sub.f90
"/tmp/bin/flang1" vla-sub.f90 -x 47 0x10000000 -opt 0 -terse 1 -inform warn -nohpf -nostatic -cmdline "'+flang -g -O0 -v vla-sub.f90'" -inform warn -disable-vectorize-pragmas -x 120 0x1000000 -debug -y 129 2 -x 19 0x400000 -quad -x 7 0x100000 -x 68 0x1 -x 59 4 -x 15 2 -x 49 0x400004 -x 51 0x20 -x 57 0x48 -x 58 0x10000 -x 124 0x1000 -tp px -x 57 0xfb0000 -x 58 0x78031040 -x 47 0x08 -x 48 4608 -x 49 0x100 -stdinc /tmp/bin/../include:/usr/local/include:/tmp/d/lib/clang/13.0.0/include:/usr/include/x86_64-linux-gnu:/include:/usr/include -def unix -def unix -def unix__ -def linux -def linux -def linux__ -def NO_MATH_INLINES -def LP64__ -def LONG_MAX=9223372036854775807L -def "SIZE_TYPE=unsigned long int" -def "PTRDIFF_TYPE=long int" -def x86_64 -def x86_64__ -def amd_64amd64__ -def k8 -def k8__ -def THROW= -def extension= -def PGLLVM__ -freeform -vect 48 -x 54 1 -x 70 0x40000000 -y 163 0xc0000000 -x 189 0x10 -stbfile vla-sub-051e06.stb -modexport vla-sub-051e06.cmod -modindex vla-sub-051e06.cmdx -output vla-sub-051e06.ilm
"/tmp/bin/flang2" vla-sub-051e06.ilm -y 129 2 -x 6 0x100 -x 42 0x400000 -y 129 4 -x 129 0x400 -y 216 1 -ieee 1 -fn vla-sub.f90 -opt 0 -terse 1 -inform warn -cmdline "'+flang -g -O0 -v vla-sub.f90'" -inform warn -disable-vectorize-pragmas -x 120 0x1000000 -debug -y 129 2 -x 68 0x1 -x 51 0x20 -x 119 0xa10000 -x 122 0x40 -x 123 0x1000 -x 127 4 -x 127 17 -x 19 0x400000 -x 28 0x40000 -x 120 0x10000000 -x 70 0x8000 -x 122 1 -x 125 0x20000 -x 164 0x800000 -quad -x 59 4 -tp px -x 120 0x1000 -x 124 0x1400 -y 15 2 -x 57 0x3b0000 -x 58 0x48000000 -x 49 0x100 -astype 0 -x 183 4 -x 121 0x800 -x 54 0x10 -x 70 0x40000000 -x 249 1023 -x 124 1 -y 163 0xc0000000 -x 189 0x10 -y 189 0x4000000 -x 183 0x10 -stbfile vla-sub-051e06.stb -asm /tmp/vla-sub-051e06.ll
"/tmp/bin/clang-12" -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations -disable-free -main-file-name vla-sub.f90 -mrelocation-model static -mframe-pointer=all -fmath-errno -fno-rounding-math -mconstructor-aliases -munwind-tables -target-cpu x86-64 -tune-cpu generic -debug-info-kind=limited -dwarf-version=4 -debugger-tuning=gdb -v -fcoverage-compilation-dir=/tmp/build.shared.d -resource-dir /tmp/lib/clang/13.0.0 -O0 -fdebug-compilation-dir=/tmp/build.shared.d -ferror-limit 19 -fgnuc-version=4.2.1 -fcolor-diagnostics -itodcalls -itodcallsbyclone -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /tmp/vla-sub-cdf94f.o -x ir /tmp/vla-sub-051e06.ll
clang -cc1 version 13.0.0 based upon LLVM 13.0.0git default target x86_64-unknown-linux-gnu
"/tmp/bin/ld.lld" -z relro --hash-style=gnu --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -o a.out /usr/lib/x86_64-linux-gnu/crt1.o /usr/lib/x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/7.5.0/crtbegin.o -L/usr/lib/gcc/x86_64-linux-gnu/7.5.0 -L/lib/x86_64-linux-gnu -L/lib/../lib64 -L/usr/lib/x86_64-linux-gnu -L/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../.. -L/home/alok/amdllvm/install/d/bin/../lib -L/lib -L/usr/lib /tmp/vla-sub-cdf94f.o -lflangmain -lflang -lflangrti -lpgmath -lquadmath -lompstub -lm -lrt -lpthread -rpath /tmp/bin/../lib -lgcc --as-needed -lgcc_s --no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed /usr/lib/gcc/x86_64-linux-gnu/7.5.0/crtend.o /usr/lib/x86_64-linux-gnu/crtn.o

Yes, I agree. I think there should be cases when even in C/C++ code this will be beneficial but not sure how frequent it shall be. We need to think about trade-off. Do you think it should be enabled only for Fortran or a command line option should be introduced to enable this otherwise it should disabled by default ?

llvm/test/DebugInfo/X86/callsitepar-fastisel.ll
12 ↗(On Diff #332870)

Sure, I shall remove this test from here.

alok updated this revision to Diff 334642.Apr 1 2021, 3:53 AM

Updated to address comments from @djtodoro

probinson added a subscriber: probinson.
probinson added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1648

I think we don't want this enabled by default at -O0. It doesn't run any optimizations that should eliminate parameters, so this is going to increase debug info size for no real benefit to the debugging experience.

dblaikie added inline comments.Apr 2 2021, 10:44 AM
clang/lib/Frontend/CompilerInvocation.cpp
1648

+1 to @probinson here. That's generally been the thinking behind why this was only enabled when optimizations are enabled. (there's some minor tradeoff here: call site descriptions in unoptimized code can still be useful if they call into optimized code, so this is a bit heuristical - assuming that all code will be compiled with the same optimization level)

If flang is in a different situation and enables some optimizations at -O0 that make call site parameter descriptions beneficial even there, then maybe we'll need some different way to differentiate optimizations V no optimizations for the purpose of this feature.

alok marked 2 inline comments as done.Apr 2 2021, 10:57 AM
alok added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1648

Thanks Paul for the comment. I shall probably add an option to enable in such cases which should remain off by default, Flang driver can pass that to compiler to turn it on.

1648

Thanks David for your explanation and thoughts. Please also look at trigger to this https://reviews.llvm.org/D99160

It is needed in a Fortran case. I shall probably drop to enable it by default and add an option which by default is disabled and Fortran driver can pass that to make use of it.

dblaikie added inline comments.Apr 2 2021, 11:12 AM
clang/lib/Frontend/CompilerInvocation.cpp
1648

Sounds like @djtodoro is suggesting in https://reviews.llvm.org/D99160#2666106 that the "Bug" to fix here is the presence of DW_OP_call_value at -O0, it seems that could be an issue for the existing/original use of call_site_parameter/call_value (in C++/clang) as it would be for flang - so the fix should probably not be flang-specific if the issue isn't flang-specific. Understanding why we produce DW_OP_call_values at -O0, whether we can/should avoid that, what it costs (how much larger the debug info gets by adding DW_TAG_call_sites at -O0) would be necessary to determine the right path forward here.

alok marked an inline comment as done.Apr 2 2021, 11:20 AM
alok added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1648

Thanks David for clarification. I shall look at it and get back.