Page MenuHomePhabricator

[Debug-Info][XCOFF] support dwarf for XCOFF for assembly output
Needs ReviewPublic

Authored by shchenz on Jan 27 2021, 5:25 AM.

Details

Summary

This patch supports dwarf debugging info for XCOFF format when clang generates an assembly output. Object output will be handled in followup patches.

Since I can not get the assembly file been assembled, I verify the debug info by checking the assembly files.

I have done some testing in debugger on the AIX system, this patch works well for some cases at 32-bit XCOFF.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > Flang.Semantics::resolve102.f90
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/test_errors.sh /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/resolve102.f90 /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/test/Semantics/Output/resolve102.f90.tmp /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Drive by comment,

Object output will be handled in followup patches.

[...]

Since I can not get the assembly file been assembled, I verify the debug info by checking the assembly files.

I think this shifts the context of the quoted text. The second part of the quoted text says that we cannot use llvm-dwarfdump to test the assembly output because an assembler that reads the assembly is not available in all LLVM build environments. The quoted text is not meant to say that the checking of the assembly is a temporary situation only until object output is added.

When support for object emission is added, please translate the empty.ll into an object + llvm-dwarfdump test. Testing the assembly output is fine for the short-term, but in the long term it causes a lot of needless test changes. The NVPTX debug-info tests are all assembly output, and they create an extra burden.

The XCOFF object output for 64-bit is some ways off. Assembly output checking will be around for some time. In any case, the assembly output should not be completely untested.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
405

I agree the Clang driver at least should produce a friendly error message for this; however, there is not a closed set of users of LLVM as a library, so the report_fatal_error is still appropriate to have.

shchenz updated this revision to Diff 323525.Fri, Feb 12, 10:37 PM
shchenz marked an inline comment as done.

1: update due to change in D95931
2: delete the function attributes in the test file

Thanks for review @jmorse and thanks for explanation @hubert.reinterpretcast

Yes, we still have some work to do for object output for XCOFF 64-bit. So before that is done, we have to test the assembly output for debug infos.
Also yes, I plan to add llvm-dwarfdump test for the newly added empty.ll when I post the object output support for DWARF of XCOFF.

llvm/test/DebugInfo/XCOFF/empty.ll
19

Good idea. Thanks for reminder.

shchenz updated this revision to Diff 323590.Sat, Feb 13, 8:12 PM

1: update due to change in D96641 and D95931

shchenz updated this revision to Diff 323753.Mon, Feb 15, 7:46 AM

1: rebase due to change in parent patch D96409

shchenz updated this revision to Diff 324181.Tue, Feb 16, 9:06 PM
shchenz added a reviewer: dblaikie.

1: typo fix

shchenz updated this revision to Diff 324221.Wed, Feb 17, 12:59 AM

1: update due to change in D96409

shchenz updated this revision to Diff 325157.Fri, Feb 19, 10:03 PM

1: rebase
2: update due to change in D96409 & D95998

shchenz updated this revision to Diff 325648.Mon, Feb 22, 7:19 PM

1: update due to change in D96409 and D95998

shchenz updated this revision to Diff 326625.Fri, Feb 26, 1:21 AM

1: make sure what we end is the .text section

llvm/include/llvm/MC/MCContext.h
276–278

The formulation of the class should fundamentally ensure that the csects and debug sections do not happen to share the same key. I don't think we should rely on the particular numeric values for that.

286–293

Add a new constructor instead of making the existing one less specific.

295–302

Suggested corresponding update.

600–606

Use Optional with a suitable enumeration type.

llvm/include/llvm/MC/MCSectionXCOFF.h
38

Same comment re: using Optional.

44–48

This is the constructor used for csects.

62–67

Suggested corresponding change.

109

Suggested corresponding change.

llvm/lib/MC/MCAsmStreamer.cpp
1435–1440

The comment only covers half the condition? That makes reading this confusing.

llvm/lib/MC/MCContext.cpp
684–688

Don't use the {} initialization for XCOFFSectionKey now that its constructors are more complicated.

llvm/lib/MC/MCSymbolXCOFF.cpp
16–18

This function (as named) is for csects. The assert should stay or some more extensive changes are needed.

30–32

This function (as named) is for csects. The assert should stay or some more extensive changes are needed.

hubert.reinterpretcast added inline comments.
llvm/test/DebugInfo/XCOFF/empty.ll
76–78

See comment about the size of the traceback table and the resulting alignment consequences.

277–279

Because the size of the traceback table is not a guaranteed to be a multiple of 4 (here it is 22 bytes), I think it may be more conservative to actually emit these labels immediately after the last code instruction (although it seems neither GCC nor XL on AIX do that). Alternatively (and GCC and XL both do so), padding should be emitted after the traceback table to maintain the alignment. @DiggerLin @jasonliu, if the second option (add padding) is chosen, I think it should be a separate patch that is a follow-up to the traceback table emission.

llvm/include/llvm/MC/MCContext.h
600–606

It may make sense to add a getXCOFFDwarfSection function that forwards to this.

llvm/lib/MC/MCAsmStreamer.cpp
2355

I tried this patch with -ffunction-sections, and the assembly I get generates a duplicate definition of L...dwline:

	.byte	3
	.byte	1
	.byte	1
	.csect .main[PR],2
L..sec_end1:

	.dwsect 0x20000
L...dwline:
	.byte	0
	.byte	5
	.byte	2
	.vbyte	4, L..sec_end1
	.byte	0
	.byte	1
	.byte	1
L..debug_line_end0:
shchenz updated this revision to Diff 327031.Sun, Feb 28, 11:07 PM
shchenz marked 11 inline comments as done.

address @hubert.reinterpretcast comments

thanks for your review @hubert.reinterpretcast . See my inline reply.

llvm/include/llvm/MC/MCContext.h
600–606

hmm, I would let this function like this for now. There would be many redundant codes between getXCOFFDwarfSection and getXCOFFSection if we create a new getXCOFFDwarfSection. Let me know if you think it is profitable to add a new function. Maybe we need a big NFC patch for this change.

llvm/lib/MC/MCAsmStreamer.cpp
2355

Good catch. Fixed in the new update.

llvm/lib/MC/MCSymbolXCOFF.cpp
30–32

I have added a FIXME in the caller of setRepresentedCsect, I will fix this later in another patch.

llvm/test/DebugInfo/XCOFF/empty.ll
76–78

Can you help to point out which comments? Do you mean we will have a functionality issue if we don't align the text section?

shchenz updated this revision to Diff 327050.Mon, Mar 1, 1:26 AM

1: handle neighbouring .loc

shchenz updated this revision to Diff 327067.Mon, Mar 1, 3:18 AM

typo fix

jasonliu added inline comments.Mon, Mar 1, 9:46 AM
llvm/include/llvm/MC/MCContext.h
600–606

I think what Hubert meant is to make a call to getXCOFFSection inside of getXCOFFDwarfSection so that you don't need to pass in so many default parameters when you just want to get the Dwarf Section. i.e. The getXCOFFDwarfSection signature could be

MCSectionXCOFF *
getXCOFFDwarfSection(StringRef Section, XCOFF::DwarfSectionSubtypeFlags DwarfSubtypeFlags, bool MultiSymbolsAllowed = true, const char *BeginSymName = nullptr);
llvm/lib/MC/MCContext.cpp
685–686

Use () to make it obvious that we are calling a constructor.

llvm/test/DebugInfo/XCOFF/empty.ll
76–78

I just meant that https://reviews.llvm.org/D95518?id=326625#inline-915279 applies here too. I'm not aware of a functionality issue with this.

llvm/include/llvm/MC/MCSectionXCOFF.h
109

Minor nit: We've been using Dwarf for camelCase.

llvm/lib/MC/MCContext.cpp
679

Minor nit: We've been using Dwarf with PascalCase.

680–682

Minor comment: == works for this check.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1804–1813

@jasonliu, I'd like your analysis of how this interacts with explicit sections.

shchenz updated this revision to Diff 327330.Mon, Mar 1, 5:19 PM
shchenz marked 4 inline comments as done.

1: format fixing

llvm/include/llvm/MC/MCContext.h
600–606

Thanks for the review @jasonliu

That would make the API more confusing in my opinion. For that case, we have getXCOFFDwarfSection and getXCOFFSection. DWARF section should also be an XCOFF section, so why can't we call getXCOFFSection directly for DWARF section? getXCOFFSection obviously already has a parameter for DWARF section?

I am a little in favor of the current implementation, if you guys insist, I will do it in another NFC patch.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1804–1813

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

llvm/test/DebugInfo/XCOFF/empty.ll
76–78

Ah, OK, I was thinking you were saying "comments" in llvm source code

jasonliu added inline comments.Mon, Mar 1, 7:49 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1804–1813

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.

llvm/test/DebugInfo/XCOFF/empty.ll
3

We should add test for -ffunction-sections, and possible explicit sections as well.

52

It might not be big deal, but it seems we are generating an extra label for no reason?

shchenz updated this revision to Diff 327366.Mon, Mar 1, 11:46 PM
shchenz marked an inline comment as done.

address @jasonliu comments:
1: handle explicit section
2: add more testcases

shchenz added inline comments.Mon, Mar 1, 11:46 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1804–1813

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)
llvm/test/DebugInfo/XCOFF/empty.ll
52

Not sure, this should be also exist without -g.

shchenz updated this revision to Diff 327373.Tue, Mar 2, 12:31 AM

some simplify to the test case

shchenz added inline comments.Tue, Mar 2, 3:03 AM
llvm/test/DebugInfo/XCOFF/empty.ll
52

Not sure, this should be also exist without -g.

What I mean is these two labels are caused by input to MC DWARF generation. The L..tmp2 is generated by a .loc directive. It will be used in dwlinesection. The L..tmp1 is generated for other usage., I have not checked into this yet. But I guess this should not be related for XCOFF DWARF.

jasonliu added inline comments.Tue, Mar 2, 11:26 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1804–1813

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:

int __attribute__((section ("explict_text_sec"))) foo() {
  return 1;
}
int bar2() {return 1;}
int __attribute__((section ("explict_text_sec"))) bar() {
  return bar2();
}

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;
}