impl -gline-tables-only #1861

rtbo · 2016-10-27T14:22:14Z

implementation of -gline-tables-only flag per #1844

LemonBoy · 2016-10-27T18:29:09Z

The tests are failing due to this llvm change.
Using auto would help the readability.

LemonBoy · 2016-10-27T21:00:47Z

gen/dibuilder.cpp

+  }
+  llvm_unreachable("unknown DebugEmissionKind");
+#else
+  return global.params.symdebug < 2 ?


This makes the level 0 emit the line informations too. As you can see here it should be possible to have the same effect by setting EmitDebugInfo to false.

Level 0 should not output any debug info, also not line numbers, so that needs to be fixed.

err... somehow i thought the OP was commenting on his own code. Sorry to duplicate your comment @LemonBoy :(

JohanEngelen · 2016-10-28T08:03:08Z

We have to be careful if we are changing the meaning of the (integer) value of global.params.symdebug, which is a parameter possibly used by the DDMD FE.

Have you thought a little about what the behavior should be of cmdline parameters and orderings like this?
-g -gline-tables-only
-g -gline-tables-only -g
-gline-tables-only -g
-gline-tables-only -gc
-gline-tables-only -gc -gline-tables-only

rtbo · 2016-10-29T09:34:25Z

In regards to cmdline parameters ordering it is the latest of the -g* params that prevail.
No change from previous behavior when there was only -g and -gc.
A warning or error would be more appropriate I guess in case of multiple -g* params given.

rtbo · 2016-10-29T13:55:34Z

there are still a few lines of code like this in driver/linker.cpp:

  // because of a LLVM bug, see LDC issue 442
  if (global.params.symdebug) {
    args.push_back("/LARGEADDRESSAWARE:NO");
  } else {
    args.push_back("/LARGEADDRESSAWARE");
  }

  // output debug information
  if (global.params.symdebug) {
    args.push_back("/DEBUG");
  }

Not sure which args must be passed to the linker with -gline-tables-only.

JohanEngelen · 2016-10-29T14:17:08Z

A warning or error would be more appropriate I guess in case of multiple -g* params given.

I think it can be helpful to allow multiple -g flags without complaining about it. I just wanted to check that you had thought about what the behavior should be (in contrast to what it is at the moment). I think Clang does the same as what you are doing btw, so should be fine.

This still needs a number of tests!

rtbo · 2016-10-30T15:56:36Z

CircleCI fails due to this change in llvm.
Unrelated to -gline-tables-only.

rtbo · 2016-10-31T09:18:28Z

This still needs a number of tests!

Tested with llvm-3.6.2 and llvm-3.9.1, this simple test do not pass

// RUN: %ldc -gline-tables-only --output-ll -of%t.ll %s && FileCheck %s < %t.ll
// Checks that ldc with -gline-tables-only do not emit debug info

int main()
{
    immutable int fact=6;
    int res=1;
    for(int i=1; i<=fact; i++) res *= i;
    return res;
}

// CHECK-NOT: DW_TAG

Also tested the 2 following tests, adapted from clang, none of them pass:

// RUN: %ldc -gline-tables-only --output-ll -of%t.ll %s && FileCheck %s < %t.ll
// Checks that ldc with "-gline-tables-only" emits metadata for
// compile unit, subprogram and file.

int main() {
  // CHECK: ret i32 0, !dbg
  return 0;
}

// CHECK: !llvm.dbg.cu = !{!0}
// CHECK: !DICompileUnit(
// CHECK: !DISubprogram(
// CHECK: !DIFile(

// RUN: %ldc -gline-tables-only --output-ll -c %s -of%t.ll && FileCheck %s < %t.ll
// Checks that ldc with "-gline-tables-only" doesn't emit debug info
// for variables and types.

// CHECK-NOT: DW_TAG_variable
int global = 42;

// CHECK-NOT: DW_TAG_typedef
// CHECK-NOT: DW_TAG_const_type
// CHECK-NOT: DW_TAG_pointer_type
// CHECK-NOT: DW_TAG_array_type
alias constCharPtrArray = const(char)*[10];

// CHECK-NOT: DW_TAG_structure_type
struct S {
  // CHECK-NOT: DW_TAG_member
  char a;
  double b;
  constCharPtrArray c;
}

// CHECK-NOT: DW_TAG_enumerator
// CHECK-NOT: DW_TAG_enumeration_type
enum E { ZERO = 0, ONE = 1 }

// CHECK-NOT: DILocalVariable
int sum(int p, int q) {
  int r = p + q;
  S s;
  E e;
  return r;
}

JohanEngelen · 2016-11-02T14:20:21Z

Need help with those tests @rtbo ?

rtbo · 2016-11-02T23:23:56Z

Need help with those tests @rtbo ?

I've figured out for the dwarf tags. tests/debuginfo/gline_tables_only2.d is still failing the same.

rtbo · 2016-11-03T08:07:39Z

Oops, cadbac0 also strips almost all metadata, which removes all effects of -gline-tables-only.

- check for type attributes absence - on LDC compile unit is not the first metadata

rtbo · 2016-11-03T22:20:30Z

Better now.
Still have noise dwarf emission for functions return types (makes debuginfo/gline_tables_only2.d fail).

rtbo · 2016-11-04T19:25:33Z

Passes with llvm-3.9 only

rtbo · 2016-11-04T21:53:24Z

Reason of test failure is that emitted metadata in IR code is far different form one version of LLVM to another.
Here is metadata emitted with llvm-3.9.1:

!llvm.module.flags = !{!0, !1}
!llvm.dbg.cu = !{!3}
!llvm.ident = !{!5}

!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = !{i32 6, !"Linker Options", !2}
!2 = !{}
!3 = distinct !DICompileUnit(language: DW_LANG_D, file: !4, producer: "LDC (http://wiki.dlang.org/LDC)", isOptimized: false, runtimeVersion: 1, emissionKind: LineTablesOnly, enums: !2)
!4 = !DIFile(filename: "factorial.d", directory: "/opt/dev/test/factorial")
!5 = !{!"ldc version e9a486"}
!6 = distinct !DISubprogram(name: "D main", linkageName: "_Dmain", scope: null, file: !4, line: 2, type: !7, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !3, variables: !2)
!7 = !DISubroutineType(types: !2)
!8 = !DILocation(line: 4, column: 5, scope: !6)
!9 = !DILocation(line: 5, column: 5, scope: !6)
!10 = !DILocation(line: 6, column: 9, scope: !11)
!11 = distinct !DILexicalBlock(scope: !6, file: !4, line: 6, column: 5)
!12 = !DILocation(line: 6, column: 5, scope: !13)
!13 = distinct !DILexicalBlock(scope: !11, file: !4, line: 6, column: 5)
!14 = !DILocation(line: 7, column: 9, scope: !13)
!15 = !DILocation(line: 8, column: 5, scope: !6)
!16 = distinct !DISubprogram(name: "__entrypoint.main", linkageName: "main", scope: null, file: !17, line: 6, type: !7, isLocal: false, isDefinition: true, scopeLine: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !3, variables: !2)
!17 = !DIFile(filename: "__entrypoint.d", directory: "/opt/dev/test/factorial")
!18 = !DILocation(line: 6, column: 15, scope: !16)
!19 = !DILocation(line: 8, column: 15, scope: !16)

and here for same code with llvm-3.6.2:

!llvm.module.flags = !{!0, !1}
!llvm.ident = !{!3}

!0 = !{i32 2, !"Debug Info Version", i32 2}
!1 = !{i32 6, !"Linker Options", !2}
!2 = !{}
!3 = !{!"ldc version e9a486"}
!4 = !MDLocation(line: 4, column: 5, scope: !5)
!5 = !{!"0x2e\00D main\00D main\00_Dmain\002\000\001\000\000\00256\000\002", !6, null, !7, null, i32 ({ i64, { i64, i8* }* })* @_Dmain, null, null, !2} ; [ DW_TAG_subprogram ] [line 2] [def] [D main]
!6 = !{!"factorial.d", !"/opt/dev/test/factorial"}
!7 = !{!"0x15\00\000\000\000\000\000\000", null, null, null, !2, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
!8 = !MDLocation(line: 5, column: 5, scope: !5)
!9 = !MDLocation(line: 6, column: 9, scope: !10)
!10 = !{!"0xb\006\005\000", !6, !5}               ; [ DW_TAG_lexical_block ] [/opt/dev/test/factorial/factorial.d]
!11 = !MDLocation(line: 6, column: 5, scope: !12)
!12 = !{!"0xb\006\005\001", !6, !10}              ; [ DW_TAG_lexical_block ] [/opt/dev/test/factorial/factorial.d]
!13 = !MDLocation(line: 7, column: 9, scope: !12)
!14 = !MDLocation(line: 8, column: 5, scope: !5)
!15 = !MDLocation(line: 6, column: 15, scope: !16)
!16 = !{!"0x2e\00__entrypoint.main\00__entrypoint.main\00main\006\000\001\000\000\00256\000\006", !17, null, !7, null, i32 (i32, i8**)* @main, null, null, !2} ; [ DW_TAG_subprogram ] [line 6] [def] [__entrypoint.main]
!17 = !{!"__entrypoint.d", !"/opt/dev/test/factorial"}
!18 = !MDLocation(line: 8, column: 15, scope: !16)

I limited the tests to llvm >= 3.9 at the moment. (Shortest path, I admit).
Is there a good reason to make more tests for other llvm versions? My point is that a regression will be seen in the llvm-3.9 tests anyway.
And are the tests for 3.9 enough in the first place?

JohanEngelen · 2016-11-05T10:14:35Z

I think testing for >=3.9 is fine.

…line-tables-only

kinke · 2016-11-07T19:21:31Z

gen/dibuilder.cpp

+typedef llvm::DIBuilder::DebugEmissionKind DebugEmissionKind;
+#endif
+
+DebugEmissionKind getDebugEmissionKind()


You may be able to get rid of the typedef (using is preferred btw) when using auto as return type here.

auto return type needs a tail expression, which makes code messy with the switch.
using is used in f65b73c.

kinke · 2016-11-07T19:22:30Z

gen/dibuilder.cpp

@@ -724,19 +777,28 @@ void ldc::DIBuilder::EmitCompileUnit(Module *m) {
  IR->module.addModuleFlag(llvm::Module::Warning, "Debug Info Version",
                           llvm::DEBUG_METADATA_VERSION);

+


Testing code vestige. Correction sneaked in 3761775.

kinke · 2016-11-07T19:23:11Z

gen/dibuilder.cpp

@@ -857,10 +920,9 @@ ldc::DISubprogram ldc::DIBuilder::EmitModuleCTor(llvm::Function *Fn,
  auto paramsArray = DBuilder.getOrCreateArray(params);
 #endif
 #if LDC_LLVM_VER >= 308
-  ldc::DISubroutineType DIFnType = DBuilder.createSubroutineType(paramsArray);
+   ldc::DISubroutineType DIFnType = DBuilder.createSubroutineType(paramsArray);


[My] clang-format disagrees; same for line 925.

kinke · 2016-11-07T19:24:50Z

gen/dibuilder.cpp

 #else
-  ldc::DISubroutineType DIFnType =
-      DBuilder.createSubroutineType(file, paramsArray);
+   ldc::DISubroutineType DIFnType = DBuilder.createSubroutineType(file, paramsArray);


Longer than 80 chars. Using auto keeps it in a single line.

kinke · 2016-11-07T19:26:38Z

gen/dibuilder.cpp

@@ -1174,7 +1236,7 @@ void ldc::DIBuilder::EmitGlobalVariable(llvm::GlobalVariable *llVar,
 }

 void ldc::DIBuilder::Finalize() {
-  if (!mustEmitDebugInfo())
+  if (!mustEmitFullDebugInfo() && !mustEmitLocationsDebugInfo())


Only !mustEmitLocationsDebugInfo() as used everywhere else?

Intent was to make obvious that Finalize must be called for all debug info cases. But yes, it comes to the same. ba5002a

kinke · 2016-11-07T19:29:48Z

tests/debuginfo/gline_tables_only2.d

+// CHECK: !DICompileUnit(
+// CHECK: !DISubprogram(
+// CHECK: !DIFile(
+// CHECK-NOT: DW_AT


Missing trailing newline (I know I'm pedantic). Are you aware that the non-existence of DW_AT is restricted to all lines following the previous !DIFile( match?

No, I was not aware. Thanks for pointing it out.
9c122f2.

JohanEngelen · 2016-11-12T19:17:01Z

Great stuff.
Nice work on the tests.

(squashed and merged)

As per #1844 .

impl -gline-tables-only (ldc-developers#1844)

189c506

fix gline-tables-only for llvm<3.9

862b96f

LemonBoy reviewed Oct 27, 2016

View reviewed changes

rtbo added 2 commits October 29, 2016 11:29

global.params.symdebug has same meaning than DDMD front end

eb8c1a9

EmitDebugInfo set to false for -gline-tables-only

5a4a844

createCompileUnit compatible with llvm <= 3.6

38e2f31

rtbo added 2 commits November 3, 2016 00:19

testing -gline_tables_only

7be6805

do not emit debuginfo with -gline-tables-only

cadbac0

rtbo added 2 commits November 3, 2016 22:57

update -gline-tables-only tests

b9aa454

- check for type attributes absence - on LDC compile unit is not the first metadata

select what debug info to emit

21991cd

rtbo added 3 commits November 4, 2016 00:41

simplify debug info selection

db7845f

oops

3ef5a3d

Do not emit subprogram type debug info

e9a486d

limit some -gline-tables-only testing to llvm>=3.9

0000333

Merge branch 'master' of https://github.com/ldc-developers/ldc into g…

088a47c

…line-tables-only

kinke mentioned this pull request Nov 7, 2016

A new beta #1862

Closed

kinke reviewed Nov 7, 2016

View reviewed changes

rtbo added 5 commits November 9, 2016 23:14

using decl instead of typedef

f65b73c

using function instead of hand crafted statement

3761775

auto and correct indentation to have 80 chars line

bd35c96

consistency in testing mustEmit*DebugInfo()

ba5002a

correct -gline-tables-only test

9c122f2

JohanEngelen merged commit 3b63eff into ldc-developers:master Nov 12, 2016

JohanEngelen mentioned this pull request Nov 12, 2016

Implement -gline-tables-only #1844

Closed

rtbo deleted the gline-tables-only branch November 12, 2016 19:59

dnadlinger pushed a commit that referenced this pull request Nov 24, 2016

Implement -gline-tables-only (PR #1861)

7b240ff

As per #1844 .

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl -gline-tables-only #1861

impl -gline-tables-only #1861

rtbo commented Oct 27, 2016

LemonBoy commented Oct 27, 2016

LemonBoy Oct 27, 2016

JohanEngelen Oct 28, 2016

JohanEngelen Oct 28, 2016

JohanEngelen commented Oct 28, 2016

rtbo commented Oct 29, 2016

rtbo commented Oct 29, 2016

JohanEngelen commented Oct 29, 2016

rtbo commented Oct 30, 2016

rtbo commented Oct 31, 2016

JohanEngelen commented Nov 2, 2016

rtbo commented Nov 2, 2016

rtbo commented Nov 3, 2016 •

edited

Loading

rtbo commented Nov 3, 2016

rtbo commented Nov 4, 2016

rtbo commented Nov 4, 2016

JohanEngelen commented Nov 5, 2016

kinke Nov 7, 2016

rtbo Nov 10, 2016

kinke Nov 7, 2016

rtbo Nov 10, 2016

kinke Nov 7, 2016 •

edited

Loading

rtbo Nov 10, 2016

kinke Nov 7, 2016 •

edited

Loading

rtbo Nov 10, 2016

kinke Nov 7, 2016

rtbo Nov 10, 2016

kinke Nov 7, 2016

rtbo Nov 10, 2016

JohanEngelen commented Nov 12, 2016

		@@ -724,19 +777,28 @@ void ldc::DIBuilder::EmitCompileUnit(Module *m) {
		IR->module.addModuleFlag(llvm::Module::Warning, "Debug Info Version",
		llvm::DEBUG_METADATA_VERSION);

impl -gline-tables-only #1861

impl -gline-tables-only #1861

Conversation

rtbo commented Oct 27, 2016

LemonBoy commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Oct 28, 2016

rtbo commented Oct 29, 2016

rtbo commented Oct 29, 2016

JohanEngelen commented Oct 29, 2016

rtbo commented Oct 30, 2016

rtbo commented Oct 31, 2016

JohanEngelen commented Nov 2, 2016

rtbo commented Nov 2, 2016

rtbo commented Nov 3, 2016 • edited Loading

rtbo commented Nov 3, 2016

rtbo commented Nov 4, 2016

rtbo commented Nov 4, 2016

JohanEngelen commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Nov 12, 2016

rtbo commented Nov 3, 2016 •

edited

Loading

kinke Nov 7, 2016 •

edited

Loading

kinke Nov 7, 2016 •

edited

Loading