In D129089#3628628, @yota9 wrote:

@ayermolo The runtime libs such as instrumentation must not depend on external libraries like libstdc++. But in our (currently internal) implementation we are using __atomic_test_and_set, I think it is better option here which eliminates inline asm usage and is arch independent

Why can't we use standard std::lock_guard here?

addressed nit

nits

In D126835#3606570, @pcwalton wrote:

The test failures in libomp don't look related to me.

In D126835#3602445, @dblaikie wrote:

In D126835#3602351, @ayermolo wrote:

In D126835#3602110, @dblaikie wrote:

In D126835#3601856, @ayermolo wrote:

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Oh, that surprises me - could you provide repro steps?

int conv32(const void* ptr) { return *(const int *) ptr; }

int foo(const void* ptr) {
  return conv32(ptr) + 2;
}

gcc -g -gstrict-dwarf -gz=none -O3 helper.cpp -c
Version 11.2.1

Huh - I'd argue that's /probably/ a bug in GCC? Describing a zero-length inlined scope seems of minimal value. I guess some very late optimization probably causes this in GCC. Might be worth filing a bug on GCC or otherwise asking them about that situation (if you do, please CC me - I'd be curious to follow along).

In D127889#3604421, @DavidSpickett wrote:

I've reverted this due to failures on our bots: https://lab.llvm.org/buildbot/#/builders/96/builds/25032/steps/6/logs/stdio

/usr/bin/ld: /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/test/Shell/Commands/Output/dwarf5-low-pc-ranges-inlining.test.tmp.dir/main5.o: Relocations in generic ELF (EM: 62)
/usr/bin/ld: /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/test/Shell/Commands/Output/dwarf5-low-pc-ranges-inlining.test.tmp.dir/main5.o: error adding symbols: file in wrong format
collect2: error: ld returned 1 exit status

I think that the system ld is expecting an AArch64 format object here. Perhaps you could use lld instead given that we always build that when running the lldb tests?

Also I'm pretty sure you'll need to require the x86 target in llvm as well. The test will fail if this is an AArch64 target only build for example.

In D126835#3602110, @dblaikie wrote:

In D126835#3601856, @ayermolo wrote:

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Oh, that surprises me - could you provide repro steps?

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Long term @zr33 (our summer intern) is working on mechanism to write out .debug_Info/.debug_abbrev using the same mechanics as in codegen/dwarflinker.
Once that is enabled we can leave this pattern as is without converting to ranges.

Still not really following - "converting to ranges" as in "changing consumers to use ranges instead of aranges"? sure - though I think that change would be a good thing in general, since aranges take up a bunch of object space.

My bad I should have been more clear.. This is just what BOLT does. We convert DW_AT_low_pc/DW_AT_high_pc to DW_AT_low_pc/DW_AT_ranges.. This is because BOLT can break up functions.
Due to the limits of our current implementation if DIEs share a an abbrev table we must convert all of them.

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

changed debug-line checks not to include offsets

got order wrong

seeing what's in line-str

Added test for split dwarf.

updated test, removed warning

Added check for .debug_line

rebase

updated test

fixed test

oops clobbered the diff

removed old code

In D126835#3579779, @dblaikie wrote:

In D126835#3577753, @hans wrote:

When trying this on Chromium (in the context of https://crbug.com/1335630) we hit a densemap assert in llvm::DwarfDebug::emitDebugARanges()
See the attached reproducer.

string_piece_unittest-c72fb0.sh12 KBDownload

string_piece_unittest-c72fb0.cpp6 MBDownload

Thanks for the reproducer, @hansw - sounds worth addressing before this moves forward.

(I'm still sort of feeling like we should fix the underlying entities to be non-zero length always (even if they're zero length at the language level - they can still be non-zero length as an implementation detail providing unique addresses))

For title maybe something like
[BOLT][DWARF] Add Unit test for DW_AT_high_pc [DW_FORM_addr]

Looks ok to me.
@dblaikie thoughts?

addressed comments

clang-format

clang-format

rebase

In D121876#3542766, @Amir wrote:

Hi,
The test started to fail on Ubuntu buildbot: https://lab.llvm.org/buildbot/#/builders/215/builds/6376

# PRECHECK: include_directories[ 0] = .debug_line_str[0x00000000]
            ^
<stdin>:8:12: note: scanning from here
 version: 5
           ^
<stdin>:30:1: note: possible intended match here
include_directories[ 0] = .debug_line_str[0x00000009] = "."

Can you please take a look?

rebase ontop of Fix LIT tests on Windows VS2019

update with arc diff git merge-base HEAD origin --update D126087

rebase

rebase

changed type unordered map to const.

Changed so that it doesn't rely on offset.

addressing comments

Hmm for some reason I wan't getting any notifications on this diff. My apologies.

In D124892#3538807, @dblaikie wrote:

In D124892#3532246, @ayermolo wrote:

In D124892#3529768, @dblaikie wrote:

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

So using this example.
When we are processing MachineFunction for main. We are processing SubProgram main, with CU in main.dwo. In constructAbstractSubprogramScopeDIE this is the SrcCU. It has debug inlining enabled.
What gets inlined in to it is Abstract Lexical Scope from helper.dwo that doesn't have debug inlining enabled.
Because of it we construct Abstract Subprogram Scope DIE only in SrcCU (Main DWO CU).

With my "fix" what happens is we end up creating CU for the helper dwo, Skeleton CU for it, and adding inline subprogram information to it for "func".
So when we construct subprogram scope die, when constructInlinedScopeDIE is invoked on scope for func (which comes from helper.cpp), we are able to find Inlined sub program and don't crash. Because we can find Origin DIE for inlined sub program.

Ideally I think we would like to have inlined sub program in main Skeleton CU. Otherwise it won't be complete.

I think that's probably where I'm suggesting differently: If someone didn't build SrcCU with split-dwarf-inlining, then even if that code gets inlined into MainCU that /does/ have split-dwarf-inlining, it would be OK/consistent /not/ to describe the inlined function from SrcCU. I think that'd be correctly respecting the level of detail the user asked for - no symbolizing for functions in (or from) SrcCU.

Could you implement that? By checking when we go to construct the inlined scope DIE if the subprogram comes from a CU without split-dwarf-inlining, and if so, skip creating the split dwarf inlined description?

refactored extractDWOTUFromDWP

In D113741#3207125, @dblaikie wrote:

In D113741#3207109, @krisb wrote:

@dblaikie, thank you for the reproducers!

I've tried to compile&examine them, but haven't found any issues. Likely, I did something wrong.
Could you, please, give more details on how to reproduce the issue(s)? I mean, exact compilation command line and/or crash details / wrong dwarf output? Thank you!

Oh, right, sorry:

For the first one, you should observe a crash (in a +Asserts build) when running this command:

$ clang++-tot ab-crash.ll -gsplit-dwarf -c -O1

The backtrace looks something like this:

$ clang++-tot ab-crash.ll -gsplit-dwarf -c -O1 -S 
clang++-tot: /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:660: llvm::DIE *llvm::DwarfCompileUnit::constructInlinedScopeDIE(llvm::LexicalScope *): Assertion `OriginDIE && "Unable to find original DIE for an inlined subprogram."' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: clang++-tot ab-crash.ll -gsplit-dwarf -c -O1 -S
1.      Code generation
2.      Running pass 'Function Pass Manager' on module 'ab-crash.ll'.
3.      Running pass 'X86 Assembly Printer' on function '@main'
 #0 0x0000000009e604ea llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:565:11
 #1 0x0000000009e6069b PrintStackTraceSignalHandler(void*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:632:1
 #2 0x0000000009e5ed63 llvm::sys::RunSignalHandlers() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Signals.cpp:97:5
 #3 0x0000000009e5fdfe llvm::sys::CleanupOnSignal(unsigned long) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:362:1
 #4 0x0000000009d6fa4e (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/CrashRecoveryContext.cpp:0:7
 #5 0x0000000009d6fe03 CrashRecoverySignalHandler(int) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/CrashRecoveryContext.cpp:390:1
 #6 0x00007f60568c08e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x138e0)
 #7 0x00007f6056389e71 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1
 #8 0x00007f6056373536 abort ./stdlib/abort.c:81:7
 #9 0x00007f605637341f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
#10 0x00007f605637341f _nl_load_domain ./intl/loadmsgcat.c:970:34
#11 0x00007f60563827f2 (/lib/x86_64-linux-gnu/libc.so.6+0x357f2)
#12 0x000000000b57ce6e llvm::DwarfCompileUnit::constructInlinedScopeDIE(llvm::LexicalScope*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:0:3
#13 0x000000000b57cb6c llvm::DwarfCompileUnit::constructScopeDIE(llvm::LexicalScope*, llvm::DIE&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:538:10
#14 0x000000000b57d3ae llvm::DwarfCompileUnit::createAndAddScopeChildren(llvm::LexicalScope*, llvm::DIE&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1072:7
#15 0x000000000b57efa1 llvm::DwarfCompileUnit::constructSubprogramScopeDIE(llvm::DISubprogram const*, llvm::LexicalScope*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1023:14
#16 0x000000000b52bc0d llvm::DwarfDebug::endFunctionImpl(llvm::MachineFunction const*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2301:50
#17 0x000000000b505ab2 llvm::DebugHandlerBase::endFunction(llvm::MachineFunction const*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp:0:5
#18 0x000000000b4e1f4c llvm::AsmPrinter::emitFunctionBody() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1505:3
#19 0x0000000007ed7113 llvm::X86AsmPrinter::runOnMachineFunction(llvm::MachineFunction&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Target/X86/X86AsmPrinter.cpp:82:3
#20 0x0000000008a96477 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:8
#21 0x0000000009165d5e llvm::FPPassManager::runOnFunction(llvm::Function&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1439:23
#22 0x000000000916abb2 llvm::FPPassManager::runOnModule(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1485:16
#23 0x0000000009166649 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1554:23
#24 0x00000000091661bd llvm::legacy::PassManagerImpl::run(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:542:16
#25 0x000000000916ae91 llvm::legacy::PassManager::run(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1681:3

But that's actually not the original situation I was investigating, so maybe it's another bug? (possibly same root cause, though)

Though the original issue I was investigating was from invalid .debug_gnu_pubnames (DIE offsets of 0, which prematurely terminates the .debug_gnu_pubnames list and produces a warning from llvm-dwarfdump), and by adding assert(Offset) in llvm::DIE::getOffset (initially I added it in the pubnames emission code, but generally it should be true that the offset of a DIE should not be queried until it's been computed, and no DIE has offset zero, because the offset is from the start of the CU contribution - so the headers come first and mean the DIE offset is always non-zero). It seems I still don't have a small reproduction for that exact issue, though.

The second test case provided above, that produces multiple DWO CUs can be reproduced as follows:

$ clang++-tot ab-multi-cu.ll -gsplit-dwarf -c -O1 && llvm-dwarfdump-tot ab-multi-cu.o
ab-multi-cu.o:  file format elf64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000053, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000057)

0x0000000b: DW_TAG_compile_unit
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_GNU_pubnames        (true)
              DW_AT_GNU_dwo_name        ("ab-multi-cu.dwo")
              DW_AT_GNU_dwo_id  (0xe9d1660e0395035d)
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x000000000000000d)
              DW_AT_GNU_addr_base       (0x00000000)

0x00000030:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000000d)
                DW_AT_name      ("main")

0x00000041:     DW_TAG_inlined_subroutine
                  DW_AT_abstract_origin (0x000000000000007b "f1")
                  DW_AT_low_pc  (0x0000000000000004)
                  DW_AT_high_pc (0x0000000000000009)
                  DW_AT_call_file       ("/usr/local/google/home/blaikie/dev/scratch/b.cpp")
                  DW_AT_call_line       (3)
                  DW_AT_call_column     (0x03)

0x00000055:     NULL

0x00000056:   NULL
0x00000057: Compile Unit: length = 0x00000027, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000082)

0x00000062: DW_TAG_compile_unit
              DW_AT_stmt_list   (0x0000004c)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_GNU_pubnames        (true)
              DW_AT_GNU_dwo_name        ("ab-multi-cu.dwo")
              DW_AT_GNU_dwo_id  (0xeb227ecffcd242c1)
              DW_AT_GNU_addr_base       (0x00000000)

0x0000007b:   DW_TAG_subprogram
                DW_AT_name      ("f1")
                DW_AT_inline    (DW_INL_inlined)

0x00000081:   NULL

In this case the second CU is OK (for the cross-CU inlining that's occurred), but it shouldn't have any DWO_* attributes, and the .dwo file shouldn't have a second CU in it (which it does after this patch) because Split DWARF doesn't have a good way to describe cross-CU references correctly once the CU is packaged in a dwp file. (this behavior is meant to be controlled by the SplitDwarfCrossCUReferences flag.

This does look like a stack trace for D124892.

In D113741#3536166, @dblaikie wrote:

In D113741#3534761, @ayermolo wrote:

In D113741#3532850, @dblaikie wrote:

In D113741#3516038, @krisb wrote:

There is an alternative implementation that relies on addition field of DISubprogram/DILexicalScope that tracks static locals, local imports and types D125693.

Ah, OK - so that 5-part sequence is an alternative to this one?

Might be good to figure out the issue here before debating which direction to go in, etc. (hmm, one of the patches in that series I've already approved? So am I reviewing alternatives here? Given the complexity here that's a bit costly for me/end up feeling like I'm drowning under trying to figure out all these moving parts... - a linear sequence is generally OK/good, I can review the easy parts and focus on the difficult ones, but multiple large alternatives can be a bit daunting)

Ping on this ^

---

In D113741#3448685, @jmorse wrote:

Drive-by comment on the topic of the gnu pubnames error,

In D113741#3439940, @krisb wrote:

If I remember correctly, this issue with .debug_gnu_pubnames was caused by another patch (https://reviews.llvm.org/rG78d15a112cbd545fbb6e1aa37c221ef5aeffb3f2).
Did I understand you right that the same issue is reproduced with w/o that patch?

I've run into that error myself when trying to alter which unit a subprogram definition goes in -- see the comment at D94976#2543257 . That patch never landed in the end because I couldn't figure out exactly what was wrong. IMHO, there's some latent weirdness that triggers that error, exactly how is unclear though. IMO, it's not specific to rG78d15a112cbd5 .

[0] "(.debug_gnu_pubnames): name lookup table at offset (...) has a terminator at offset (...) before the expected end at (...)"

I've tried to reproduce the issue using the example from D94976 (and with this patch), it compiled fine and didn't show the mentioned problem with pubnames.
Another question is about the combination of FullLTO and split-dwarf which has been already discussed in this thread and which isn't (fully) supported by LLVM mainline (yet, at least in a way that doesn't violate DWARF specification).

So, I'm still looking for a reproducer for the issue with pubnames. Any help will be appreciated.

Yeah, I still need to figure out how to do that/get you that. :/

(is this at all/possibly related to https://reviews.llvm.org/D124892 ?)

Seems like a discussion is more about multiple CUs in DWO with full LTO? Need to read all the history in more details.
Side note BOLT doesn't support multiple CUs in the dwo. There are certain assumptions about abbrev, and how it writes out new dwo files.
Does pubnames actually used for anything? My impression is that debuggers (or at least lldb) ignores it.

Yep, debug_gnu_pubnames are used by gold and lld to build gdb_index which improves gdb startup time.

formating

Addressing comments.

In D113741#3532850, @dblaikie wrote:

In D113741#3516038, @krisb wrote:

There is an alternative implementation that relies on addition field of DISubprogram/DILexicalScope that tracks static locals, local imports and types D125693.

Ah, OK - so that 5-part sequence is an alternative to this one?

Might be good to figure out the issue here before debating which direction to go in, etc. (hmm, one of the patches in that series I've already approved? So am I reviewing alternatives here? Given the complexity here that's a bit costly for me/end up feeling like I'm drowning under trying to figure out all these moving parts... - a linear sequence is generally OK/good, I can review the easy parts and focus on the difficult ones, but multiple large alternatives can be a bit daunting)

---

In D113741#3448685, @jmorse wrote:

Drive-by comment on the topic of the gnu pubnames error,

In D113741#3439940, @krisb wrote:

If I remember correctly, this issue with .debug_gnu_pubnames was caused by another patch (https://reviews.llvm.org/rG78d15a112cbd545fbb6e1aa37c221ef5aeffb3f2).
Did I understand you right that the same issue is reproduced with w/o that patch?

I've run into that error myself when trying to alter which unit a subprogram definition goes in -- see the comment at D94976#2543257 . That patch never landed in the end because I couldn't figure out exactly what was wrong. IMHO, there's some latent weirdness that triggers that error, exactly how is unclear though. IMO, it's not specific to rG78d15a112cbd5 .

[0] "(.debug_gnu_pubnames): name lookup table at offset (...) has a terminator at offset (...) before the expected end at (...)"

I've tried to reproduce the issue using the example from D94976 (and with this patch), it compiled fine and didn't show the mentioned problem with pubnames.
Another question is about the combination of FullLTO and split-dwarf which has been already discussed in this thread and which isn't (fully) supported by LLVM mainline (yet, at least in a way that doesn't violate DWARF specification).

So, I'm still looking for a reproducer for the issue with pubnames. Any help will be appreciated.

Yeah, I still need to figure out how to do that/get you that. :/

(is this at all/possibly related to https://reviews.llvm.org/D124892 ?)

missed llvm-dwp

In D124892#3529768, @dblaikie wrote:

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

Please update the title with [BOLT][DWARF] <title>

@dblaikie Maybe you can point me in a direction where the fix could be better implemented? I can see what I can come up with.