This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Change FindDefinitionTypeForDWARFDeclContext() to take DWARFDIE
ClosedPublic

Authored by aeubanks on Nov 23 2022, 2:30 PM.

Diff Detail

Event Timeline

aeubanks created this revision.Nov 23 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald Transcript
aeubanks requested review of this revision.Nov 23 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 2:30 PM
labath accepted this revision.Nov 24 2022, 2:12 AM
This revision is now accepted and ready to land.Nov 24 2022, 2:12 AM
This revision was landed with ongoing or failed builds.Nov 24 2022, 8:40 AM
This revision was automatically updated to reflect the committed changes.

Hi Arthur, this change is causing a testsuite failure on Darwin systems - see https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48671/ on the "LLDB Incremental" bot - and I can reproduce this failure on my local mac. Would you like to revert this until you've had a chance to find the issue, or should I? It's a holiday weekend in the US & I would understand if you don't see this message for a while, I'll revert later today unless you suggest an alternative. Thanks.

Hi Arthur, this change is causing a testsuite failure on Darwin systems - see https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48671/ on the "LLDB Incremental" bot - and I can reproduce this failure on my local mac. Would you like to revert this until you've had a chance to find the issue, or should I? It's a holiday weekend in the US & I would understand if you don't see this message for a while, I'll revert later today unless you suggest an alternative. Thanks.

Feel free to revert, but could you post the log file that the test looks at with and without the change?

Hi Arthur, this change is causing a testsuite failure on Darwin systems - see https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48671/ on the "LLDB Incremental" bot - and I can reproduce this failure on my local mac. Would you like to revert this until you've had a chance to find the issue, or should I? It's a holiday weekend in the US & I would understand if you don't see this message for a while, I'll revert later today unless you suggest an alternative. Thanks.

Feel free to revert, but could you post the log file that the test looks at with and without the change?

The bot is also broken with Adrian Vogelsgesang's coroutines formatter right now and I was going to wait until tomorrow morning US time before I revert his patchsets, not sure which time zone he's in, so I won't do anything until then at least.

The bot has pretty good logs fwiw,

FAIL: test_dwarf (TestCPPAccelerator.CPPAcceleratorTableTestCase)
   Test that type lookups fail early (performance)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1691, in test_method
    return attrvalue(self)
  File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 159, in wrapper
    return func(*args, **kwargs)
  File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/lang/cpp/accelerator-table/TestCPPAccelerator.py", line 32, in test
    self.assertEqual(n, 1, "".join(log))
AssertionError: 0 != 1 :  (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: DWARFASTParserClang::ParseTypeFromDWARF (die = 0x00000127, decl_ctx = 0x7fc72ebdb430 (die 0x0000000b)) DW_TAG_reference_type name = '(null)')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: DWARFASTParserClang::ParseTypeFromDWARF (die = 0x0000012c, decl_ctx = 0x7fc72ebdb430 (die 0x0000000b)) DW_TAG_class_type name = 'D')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: DWARFASTParserClang::ParseTypeFromDWARF (die = 0x00000142, decl_ctx = 0x7fc72ebdbca0 (die 0x0000012c)) DW_TAG_structure_type name = 'Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: SymbolFileDWARF(0x7fc72c4a7a50) - 0x00000142: DW_TAG_structure_type type "Inner" is a forward declaration, trying to find complete type
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/a.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/a.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner') trying die=0x00000061 (A::Inner)
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/b.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/b.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner') trying die=0x00000061 (B::Inner)
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/c.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/c.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner') trying die=0x00000061 (C::Inner)
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/d.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/d.o: SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(tag=DW_TAG_structure_type, name='Inner') trying die=0x00000061 (D::Inner)
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/d.o: DWARFASTParserClang::ParseTypeFromDWARF (die = 0x0000004b, decl_ctx = 0x7fc72ebdb430 (die 0x0000000b)) DW_TAG_class_type name = 'D')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/d.o: DWARFASTParserClang::ParseTypeFromDWARF (die = 0x00000061, decl_ctx = 0x7fc72ebdbca0 (die 0x0000004b)) DW_TAG_class_type name = 'Inner')
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/main.o: SymbolFileDWARF(0x7fc72c4a7a50) - 0x00000142: DW_TAG_structure_type type "Inner" is a forward declaration, complete type is 0x500000061
 (x86_64) /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/lang/cpp/accelerator-table/TestCPPAccelerator.test_dwarf/d.o: DWARFASTParserClang::ParseTypeFromDWARF (die = 0x0000007e, decl_ctx = 0x7fc72ebdb430 (die 0x0000000b)) DW_TAG_base_type name = 'int')

I didn't look into this patch enough to say whether this is a real failure, or if the test assumes behavior that is only true with the accelerator tables used in DWARF on linux, the apple ones used on macOS. In which case this may simply be a skipIfDarwin test, I don't know.

I didn't look into this patch enough to say whether this is a real failure, or if the test assumes behavior that is only true with the accelerator tables used in DWARF on linux, the apple ones used on macOS. In which case this may simply be a skipIfDarwin test, I don't know.

If this is a difference in behavior between the DWARF accelerator tables used on linux versus the apple accelerator tables used on macOS, it's an interesting question of whether this is pointing out a bug in the apple accelerator table support in lldb or something to do with how the tables are set up. If you don't have access to a macOS system to investigate more closely, and you don't think this is indicating a bug or regression in behavior, then I think skipping the test for now on darwin is not unreasonable. My two cents having not looked into any of these changes.

Reverted for now.

commit 1df47dbe131d2aed676d9d5e0441ff4b61fec018
Author: Jason Molenda <jason@molenda.com>
Date:   Fri Nov 25 12:11:37 2022 -0800

    Revert "[lldb][NFC] Change FindDefinitionTypeForDWARFDeclContext() to take DWARFDIE"
    
    The changes in https://reviews.llvm.org/D138612 cause a testsuite
    failure on Darwin systems in TestCPPAccelerator.py, first flagged
    by the "LLDB Incremental" CI bot.
    https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/
    with the specific failure text appended to the phabracator.
    
    This reverts commit c3c423b6cb2e1c00483e951ce8cc4d935e31cd14.

FWIW when I was commenting on this last night, I hadn't realized that the test case was unmodified by the patch. Reverting the patch until we can figure out the cause of the behavior change made the most sense. The macOS incremental bot is green now.

The bot has pretty good logs fwiw,

I don't have easy access to a mac, knowing what the logs are when the test is passing would be helpful with figuring out what's going wrong and relanding.

aeubanks reopened this revision.Nov 28 2022, 9:37 AM
This revision is now accepted and ready to land.Nov 28 2022, 9:37 AM
aeubanks updated this revision to Diff 478272.Nov 28 2022, 9:46 AM

potential test fix

aeubanks retitled this revision from [lldb][NFC] Change FindDefinitionTypeForDWARFDeclContext() to take DWARFDIE to [lldb] Change FindDefinitionTypeForDWARFDeclContext() to take DWARFDIE.Nov 28 2022, 9:47 AM

@jasonmolenda could you run check-lldb with this patch? I think this patch may fix the test by changing its expectations for logging.

(removed "NFC" from title because the logging changes actually mattered)

aeubanks updated this revision to Diff 479056.Nov 30 2022, 1:16 PM

go back to calling m_index->GetTypes(DWARFDeclContext) instead of m_index->GetTypes(StringRef)

these have different codepaths, I mistakenly thought they were equivalent

I borrowed a mac and verified that this patch fixes the test

This revision was landed with ongoing or failed builds.Nov 30 2022, 1:20 PM
This revision was automatically updated to reflect the committed changes.