[PATCH] D95518: [Debug-Info][XCOFF] support dwarf for XCOFF for assembly output

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1804-1807
+  // When -function-sections is enabled, generating function section end symbol
+  // to tell debug line section where is the end of current function section.
+  if (TM.getFunctionSections() && !MAI->usesDwarfFileAndLocDirectives() &&
+      MMI->hasDebugInfo())
----------------
jasonliu wrote:
> shchenz wrote:
> > jasonliu wrote:
> > > shchenz wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > @jasonliu, I'd like your analysis of how this interacts with explicit sections.
> > > > `explicit sections` are generated when `-fdata-sections` is enabled? If so, I think debugging functionality should not be impacted. We only need to worry about the "text" section for debug line. For `explicit sections` functionality, I guess it should not be impacted too as we only add new labels, not data, like `vbyte`, `byte`
> > > > 
> > > > Anyway, your input is appreciated @jasonliu 
> > > It seems that we want to generate a section end label on every text csect here.
> > > If that's the case, then it doesn't seem like we are generating them correctly when explicit section for text section is enabled. An example would be:
> > > ```
> > > int bar() {return 1;}
> > > int __attribute__((section ("explicit_main_sec"))) main() {
> > >   return bar();
> > > }
> > > ```
> > > I don't see a `L..sec_end` for `.text[PR]`.
> > > 
> > > A side note when I was trying things: it seems we have a subtle bug when we enables -function-sections and specify an explicit section for a function. We don't generate a label for the function entry point in this case, and we don't have the correct function entry point stored in the function descriptor section. 
> > > The bug is not related to this patch, and we need to fix it in another patch. But it affects how we want to construct a test case in this patch.
> > Good catch. We need to generate section end symbol for the explicit section too. Fixed in the new patch.
> > 
> > I found one issue, not sure if it is the same one as you found:
> > for the case you given, if we compile it with `-ffunction-sections`, we will get some linking error:
> > ```
> > > clang 4.c -ffunction-sections          
> > ld: 0711-317 ERROR: Undefined symbol: .main
> > ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
> > clang-13: error: linker command failed with exit code 8 (use -v to see invocation)
> > ```
> Yes, it's the same bug.
> I feel the current way of handling end of text csect is not very robust.
> A few more ways to break it:
> 1.
> ```
> int __attribute__((section ("explict_text_sec"))) foo() {
>   return 1;
> }
> int bar2() {return 1;}
> int __attribute__((section ("explict_text_sec"))) bar() {
>   return bar2();
> }
> 
> ```
> 2. 
> It seems when C++ exception handling is enabled, and eh instruments are generated, then we will not have the label `L..sec_end` on the correct location. 
> A sample program would be:
> ```
> int foo(){
>   try { throw 1;}
>   catch(...) { return 1;}
>   return 0;
> }
> ```
issue 1 is indeed an issue. Fixed in the new patch. Now, I use a new method NOW. Use end symbol of `.text` section for all sections including any function sections or explicit sections.

issue 2 is as expected, we have to contain this inaccurate text end symbol, because we may generate other sections (maybe not for text, for example, eh, rodata sections) between functions, and we need an end symbol for all instructions in the current CU, so we have to add it in `PPCAIXAsmPrinter::doFinalization()` which runs after all functions being handled by `AsmPrinter::runOnMachineFunction()`. But this should not impact debugability as the inaccurate label only impact the last debug line entry in the current CU and this inaccurate label is out of any function, so before we reach this address in the debugger, we should jump to the caller of the last function in the section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518