[PATCH] D79978: Call Frame Information (CFI) Handling for Basic Block Sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/DebugInfo/X86/basicblock-sections-cfi.ll:4-10
+; From:
+; int foo(int a) {
+;   if (a > 20)
+;     return 2;
+;   else
+;     return 0;
+; }
----------------
tmsriram wrote:
> dblaikie wrote:
> > I /might/ find this simpler:
> > 
> > ```
> > void f1();
> > void f2();
> > void f3(bool b) {
> >   if (b)
> >     f1();
> >   else
> >     f2();
> > }
> > ```
> > 
> > But no big deal either way - maybe some more exposition on why 1 CIE and 4FDEs are expected? (I guess 1 CIE shares some common data across the 4 FDEs which are for the 4 basic blocks? Could it be fewer, what about "void f1(); void f2(bool b) { if (b)  f1(); }" is that still adequate to test this functionality?)
> @amharc 
> 
> Thanks for taking a look, I will change the code.
> 
> The FDEs cannot be fewer, one is needed per section.  This is because FDEs do not support ranges like debug info and it is only low-high PC.  In theory, each section could be in arbitrary non-contiguous locations and hence conservatively we need four.
> 
> We have a CFI dedup that will reduce the size of each FDE and move the common parts out to the CIE.  I split that out of this patch to keep this simple.
> 
> 
> > Could it be fewer, what about "void f1(); void f2(bool b) { if (b) f1(); }" is that still adequate to test this  functionality?)
> 
> This will not capture the deduping well because we need atleast 4 basic blocks to have duplicates.  The entry and the exit block have other CFI instructions.
> 
> 
> We have a CFI dedup that will reduce the size of each FDE and move the common parts out to the CIE. I split that out of this patch to keep this simple.

Then I'd generally suggest keeping this test simple too - though I can understand the benefit of having the diff on the test just demonstrate how things got better in the later change (rather than introducing test changes or new tests that demonstrate the new behavior). Your call.


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

https://reviews.llvm.org/D79978