This is an archive of the discontinued LLVM Phabricator instance.

Adding test for DIMacro and DIMacroFile comparison implementation
ClosedPublic

Authored by aaboud on Aug 2 2016, 2:54 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 66447.Aug 2 2016, 2:54 AM
aaboud retitled this revision from to Adding test for DIMacro and DIMacroFile comparison implementation.
aaboud updated this object.
aaboud added a reviewer: dblaikie.
aaboud added subscribers: llvm-commits, bwyma.
dblaikie edited edge metadata.Aug 2 2016, 9:32 AM

Thanks for writing up the test!

Though this looks like a few too many CHECK lines - we usually don't check the entire contents of the output like that, so that the test is resilient to unrelated changes.

Perhaps you could show/attach the output before and after the change, so we could see what particular part changed & decide how to test just that part or the surrounding important parts?

aaboud added a comment.Aug 2 2016, 9:37 AM

The output after the fix appears as is in the "; CHECK" lines in the LIT tests.
And here is the output before:

!llvm.dbg.cu = !{!0, !11}
!llvm.ident = !{!16, !16}
!llvm.module.flags = !{!17}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0 (trunk 276746)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, macros: !3)
!1 = !DIFile(filename: "t.c", directory: "/")
!2 = !{}
!3 = !{!4, !5}
!4 = !DIMacro(type: DW_MACINFO_undef, line: 1, name: "X")
!5 = !DIMacroFile(line: 2, file: !6, nodes: !7)
!6 = !DIFile(filename: "t.h", directory: "/")
!7 = !{!8, !9}
!8 = !DIMacro(type: DW_MACINFO_define, line: 3, name: "X", value: "5")
!9 = !DIMacroFile(line: 4, file: !10)
!10 = !DIFile(filename: "t2.h", directory: "/")
!11 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0 (trunk 276746)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, macros: !12)
!12 = !{!4, !13}
!13 = !DIMacroFile(line: 2, file: !6, nodes: !14)
!14 = !{!8, !15}
!15 = !DIMacroFile(line: 4, file: !10)
!16 = !{!"clang version 4.0.0 (trunk 276746)"}
!17 = !{i32 2, !"Debug Info Version", i32 3}

Ah, thanks - OK, so the issue was that the second DICompileUnit ended up sharing the first's DIMacroFile.
Does the test require both a top level DIMacro and a nested DIMacroFile+DIMacro? Or could a difference be observed with a smaller difference in just a DIMacro, etc?

So CHECK lines something like this would emphasize that:

; Verify that DICompileUnits don't share DIMacro info even though they are different
; CHECK: DICompileUnit
; CHECK-SAME: macros: [[MS1:![0-9]*]]
; CHECK: [[MS1]] = !{[[MF1:![0-9]*]]}
; CHECK: [[MF1]] = !DIMacroFile(
; CHECK-SAME: file: [[F1:![0-9]*]]
; CHECK: [[F1]] = !DIFile(filename: "x.h"
; CHECK: DICompileUnit
; CHECK-SAME: macros: [[MS2:![0-9]*]]
; CHECK: [[MS2]] = !{[[MF2:![0-9]*]]}
; CHECK: [[MF2]] = !DIMacroFile(
; CHECK-SAME: file: [[F2:![0-9]*]]
; CHECK: [[F2]] = !DIFile(filename: "y.h"

Side note: Is there any reason we're creating DIMacroFiles that have no macros in them? Or are they unnecessary?

aaboud added a comment.Aug 4 2016, 8:55 AM

Side note: Is there any reason we're creating DIMacroFiles that have no macros in them? Or are they unnecessary?

  1. GCC does that too.
  2. It is implementation details, because we create the DIMacroFile before we parse its content, thus at that point we do not know if it will contain macros or not.
aaboud added a comment.Aug 6 2016, 1:39 PM

Ah, thanks - OK, so the issue was that the second DICompileUnit ended up sharing the first's DIMacroFile.

I just noticed that you got me wrong.
The issue is that the second DICimpileUnit ended up not sharing the first's DIMacroFile.

Anyway, I will try improve the test using the above as an example.

aaboud updated this revision to Diff 67336.Aug 9 2016, 7:09 AM
aaboud edited edge metadata.

Reduced and simplified the LIT test.

dblaikie accepted this revision.Aug 9 2016, 10:37 AM
dblaikie edited edge metadata.
This revision is now accepted and ready to land.Aug 9 2016, 10:37 AM
This revision was automatically updated to reflect the committed changes.