This is an archive of the discontinued LLVM Phabricator instance.

Differential D85414

[test][DebugInfo] Adapt two tests for Sun assembler syntax on Sparc
ClosedPublic

Authored by ro on Aug 6 2020, 2:29 AM.

Details

Reviewers
echristo
jhenderson
dblaikie
MaskRay
Commits
rG0b90a08f7722: [test][DebugInfo] Adapt two tests for Sun assembler syntax on Sparc
Summary

Two DebugInfo tests currently FAIL on Sparc:

LLVM :: DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll
LLVM :: DebugInfo/Generic/array.ll

both in a similar way. E.g.

: 'RUN: at line 1';   /var/llvm/local-sparcv9-A/bin/llc -O2 /vol/llvm/src/llvm-project/local/llvm/test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll -o - | /var/llvm/local-sparcv9-A/bin/FileCheck /vol/llvm/src/llvm-project/local/llvm/test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll

/vol/llvm/src/llvm-project/local/llvm/test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll:4:10: error: CHECK: expected string not found in input
; CHECK: debug_info,
         ^

On amd64-pc-solaris2.11, the corresponding line is

.section        .debug_info,"",@progbits

while on sparcv9-sun-solaris2.11 we have only

.section        .debug_info

This happens because Sparc currently emits .section directives using the style of the Solaris/SPARC assembler (controlled by SunStyleELFSectionSwitchSyntax).

This patch takes the easy way out and allows both forms.

Tested on sparcv9-sun-solaris2.11 and amd64-pc-solaris2.11.

Alternatively, one could think about changing the default to the common GNU as
syntax instead to avoid issues like this one. I've a separate patch that goes that route, to be submitted shortly.

Diff Detail

Event Timeline

ro created this revision.Aug 6 2020, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 2:29 AM
Herald added subscribers: fedor.sergeev, jyknight. · View Herald Transcript
ro requested review of this revision.Aug 6 2020, 2:29 AM
Harbormaster completed remote builds in B67282: Diff 283535.Aug 6 2020, 2:42 AM
jhenderson added inline comments.Aug 6 2020, 2:43 AM
llvm/test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll
4

Unless you think it's important to verify the name exactly ends with "debug_info" (note that this line doesn't check that the name starts with "debug_info", so could pass with e.g. ".some_random_text_debug_info"), you can just get rid of the ',' instead.

Honestly, I'm not really sure what the point of checking the line is at all... It doesn't prove that the next two lines are in the .debug_info section, because a) the first check doesn't actually check the full section directive to show that it's referring to a section directive, and b) there could be anything, including other section directives between the check and the lines below... Same goes with the other test. I'm not sure if it's worth fixing things further thought...

ro mentioned this in D85415: [Sparc] Don't use SunStyleELFSectionSwitchSyntax.Aug 6 2020, 2:45 AM
ro added a comment.Aug 6 2020, 2:52 AM

FWIW, I've now posted the alternative patch to switch Sparc to the common ELF .section syntax instead: D85415. If we go this way, the current patch becomes moot.

llvm/test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll
4

Unless you think it's important to verify the name exactly ends with "debug_info" (note that this line doesn't check that the name starts with "debug_info", so could pass with e.g. ".some_random_text_debug_info"), you can just get rid of the ',' instead.

Removing the , was my first thought. However, there are a couple of lines matching just debug_info:

	.section	.debug_info
	.word	.Ldebug_info_end0-.Ldebug_info_start0 ! Length of Unit
.Ldebug_info_start0:
.Ldebug_info_end0:

Honestly, I'm not really sure what the point of checking the line is at all... It doesn't prove that the next two lines are in the .debug_info section, because a) the first check doesn't actually check the full section directive to show that it's referring to a section directive, and b) there could be anything, including other section directives between the check and the lines below... Same goes with the other test. I'm not sure if it's worth fixing things further thought...

I'd been wary of tightening the check because I'm not sure which systems with non-ELF assembler syntaxes might be affected (e.g. Darwin), so this felt like a can of worms to me.

jhenderson added a comment.Aug 6 2020, 2:55 AM

What do you think about just deleting the line entirely?

ro added a comment.Aug 6 2020, 2:57 AM
In D85414#2199222, @jhenderson wrote:

What do you think about just deleting the line entirely?

Fine with me, of course. However, I'm anything but a DWARF expert...

jhenderson added a comment.Aug 6 2020, 3:03 AM

I'm not really familiar with these tests or underlying code. Let's hold off and let others have a chance to chip in with their opinions.

dblaikie added a comment.Aug 6 2020, 6:04 PM

If we're talking about perfection - the best thing to do would be to upgrade these tests to use llvm-dwarfdump instead of raw assembly testing.

If we're talking about just "let's make this work" - is the "section" part consistent on all platforms? Could we extend the check lines to be more like: "# CHECK: .section .debug_info" ? I'm not entirely sure. If we can't easily be sure of the answer to that, I'd probably go with the patch as-is.

Removing the debug_info CHECK line risks these checks matching the abbrev contents, instead of the debug_info contents - which would be a slight regression in the intentions of tnhe test, though probably not a huge one.

MaskRay added a comment.Aug 6 2020, 8:55 PM

.section is consistent on ELF, COFF, Mach-O and wasm. We could use .section {{.*}}debug_info (On Mach-O it is .section __DWARF,__debug_info,regular,debug)

ro updated this revision to Diff 283835.Aug 7 2020, 1:17 AM

Adapt checks following MaskRay's suggestion.

ro added a comment.Aug 7 2020, 1:20 AM
In D85414#2201806, @MaskRay wrote:

.section is consistent on ELF, COFF, Mach-O and wasm. We could use .section {{.*}}debug_info (On Mach-O it is .section __DWARF,__debug_info,regular,debug)

This works fine indeed (tested on sparcv9-sun-solaris2.11 with both the Sun and common ELF section syntax, amd64-pc-solaris2.11, x86_64-pc-linux-gnu, and x86_64-apple-darwin20.0.0, here using the llc from the 11.0.0 rc1 tarball and running FileCheck on a different system).

It has the additional advantage of not requiring any special handling for the Sun section syntax that may well go away.

Harbormaster completed remote builds in B67430: Diff 283835.Aug 7 2020, 1:57 AM
dblaikie accepted this revision.Aug 7 2020, 12:11 PM

Looks good to me - thanks!

This revision is now accepted and ready to land.Aug 7 2020, 12:11 PM
Closed by commit rG0b90a08f7722: [test][DebugInfo] Adapt two tests for Sun assembler syntax on Sparc (authored by ro). · Explain WhyAug 8 2020, 12:14 AM
This revision was automatically updated to reflect the committed changes.
ro added a commit: rG0b90a08f7722: [test][DebugInfo] Adapt two tests for Sun assembler syntax on Sparc.
ro mentioned this in rGd9993484ee40: [Sparc] Don't use SunStyleELFSectionSwitchSyntax.Aug 17 2022, 4:00 AM

Revision Contents

PathSize
llvm/
test/
DebugInfo/
Generic/
2 lines
2 lines

Diff 284121

llvm/test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll

; RUN: %llc_dwarf -O2 %s -o - | FileCheck %s ; RUN: %llc_dwarf -O2 %s -o - | FileCheck %s
; Check struct X for dead variable xyz from inlined function foo. ; Check struct X for dead variable xyz from inlined function foo.
; CHECK: debug_info, ; CHECK: .section {{.*}}debug_info
jhendersonUnsubmitted
Not Done
ReplyInline Actions

Unless you think it's important to verify the name exactly ends with "debug_info" (note that this line doesn't check that the name starts with "debug_info", so could pass with e.g. ".some_random_text_debug_info"), you can just get rid of the ',' instead.

Honestly, I'm not really sure what the point of checking the line is at all... It doesn't prove that the next two lines are in the .debug_info section, because a) the first check doesn't actually check the full section directive to show that it's referring to a section directive, and b) there could be anything, including other section directives between the check and the lines below... Same goes with the other test. I'm not sure if it's worth fixing things further thought...

jhenderson: Unless you think it's important to verify the name exactly ends with "debug_info" (note that…
roAuthorUnsubmitted
Done
ReplyInline Actions

Unless you think it's important to verify the name exactly ends with "debug_info" (note that this line doesn't check that the name starts with "debug_info", so could pass with e.g. ".some_random_text_debug_info"), you can just get rid of the ',' instead.

Removing the , was my first thought. However, there are a couple of lines matching just debug_info:

	.section	.debug_info
	.word	.Ldebug_info_end0-.Ldebug_info_start0 ! Length of Unit
.Ldebug_info_start0:
.Ldebug_info_end0:

Honestly, I'm not really sure what the point of checking the line is at all... It doesn't prove that the next two lines are in the .debug_info section, because a) the first check doesn't actually check the full section directive to show that it's referring to a section directive, and b) there could be anything, including other section directives between the check and the lines below... Same goes with the other test. I'm not sure if it's worth fixing things further thought...

I'd been wary of tightening the check because I'm not sure which systems with non-ELF assembler syntaxes might be affected (e.g. Darwin), so this felt like a can of worms to me.

ro: > Unless you think it's important to verify the name exactly ends with "debug_info" (note that…
; CHECK: DW_TAG_structure_type ; CHECK: DW_TAG_structure_type
; CHECK-NEXT: DW_AT_name ; CHECK-NEXT: DW_AT_name
source_filename = "test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll" source_filename = "test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll"
@i = common global i32 0, !dbg !0 @i = common global i32 0, !dbg !0
; Function Attrs: nounwind readnone ; Function Attrs: nounwind readnone
▲ Show 20 Lines • Show All 52 Lines • Show Last 20 Lines

llvm/test/DebugInfo/Generic/array.ll

Show All 19 Lines
!2 = distinct !DICompileUnit(language: DW_LANG_C99, producer: "clang version 3.0 (trunk 129138)", isOptimized: false, emissionKind: FullDebug, file: !14, enums: !15, retainedTypes: !15, imports: null) !2 = distinct !DICompileUnit(language: DW_LANG_C99, producer: "clang version 3.0 (trunk 129138)", isOptimized: false, emissionKind: FullDebug, file: !14, enums: !15, retainedTypes: !15, imports: null)
!3 = !DISubroutineType(types: !4) !3 = !DISubroutineType(types: !4)
!4 = !{!5} !4 = !{!5}
!5 = !DIBasicType(tag: DW_TAG_base_type, name: "int", size: 32, align: 32, encoding: DW_ATE_signed) !5 = !DIBasicType(tag: DW_TAG_base_type, name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
!6 = !DILocalVariable(name: "a", line: 4, scope: !7, file: !1, type: !8) !6 = !DILocalVariable(name: "a", line: 4, scope: !7, file: !1, type: !8)
!7 = distinct !DILexicalBlock(line: 3, column: 12, file: !14, scope: !0) !7 = distinct !DILexicalBlock(line: 3, column: 12, file: !14, scope: !0)
!8 = !DICompositeType(tag: DW_TAG_array_type, align: 32, file: !14, scope: !2, baseType: !5, elements: !9) !8 = !DICompositeType(tag: DW_TAG_array_type, align: 32, file: !14, scope: !2, baseType: !5, elements: !9)
!9 = !{!10} !9 = !{!10}
;CHECK: debug_info, ;CHECK: .section {{.*}}debug_info
;CHECK: DW_TAG_subrange_type ;CHECK: DW_TAG_subrange_type
;CHECK-NEXT: DW_AT_type ;CHECK-NEXT: DW_AT_type
;CHECK-NOT: DW_AT_lower_bound ;CHECK-NOT: DW_AT_lower_bound
;CHECK-NOT: DW_AT_upper_bound ;CHECK-NOT: DW_AT_upper_bound
;CHECK-NEXT: End Of Children Mark ;CHECK-NEXT: End Of Children Mark
!10 = !DISubrange(count: -1) !10 = !DISubrange(count: -1)
!11 = !DILocation(line: 4, column: 7, scope: !7) !11 = !DILocation(line: 4, column: 7, scope: !7)
!12 = !DILocation(line: 5, column: 3, scope: !7) !12 = !DILocation(line: 5, column: 3, scope: !7)
!14 = !DIFile(filename: "array.c", directory: "/private/tmp") !14 = !DIFile(filename: "array.c", directory: "/private/tmp")
!15 = !{} !15 = !{}
!16 = !{i32 1, !"Debug Info Version", i32 3} !16 = !{i32 1, !"Debug Info Version", i32 3}