Add section in SourceLevelDebugging.rst with information
about debugify and how to use it in regression tests.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 20130 Build 20130: arc lint + arc unit
Event Timeline
docs/SourceLevelDebugging.rst | ||
---|---|---|
1482 | "non null" is implied here, please omit it. | |
1485 | Could you add a section explaining what kind of debug information is attached? Specifically: instructions are assigned sequentially-increasing line locations, and are immediately used by debug value intrinsics when possible. It would help if the first example is a comparison of a short function before/after -debugify is applied. | |
1486 | This is a good example, but it should come after the one which shows a before/after with -debugify. Please explain what the error message means and how (at a high level) to fix it. You also only really need one ERROR line here, please omit the rest. | |
1527 | Please show an example of using -debugify to test a backend, e.g: | |
1540 | I think it'd be better to use a real, concrete example here. Please find one with stricter CHECK lines from the perspective of the actual optimization (e.g check the operands of the mul). | |
1551 | Please justify the '2' here to readers (btw why isn't it 1?). It's worth adding a note that checking for a DILocation in this way is brittle. If someone updating the test inserts an instruction in the original IR before the second instruction, the test will break. Discuss the tradeoff between having a brittle test, and a test that is stringent enough to check for correct debug location preservation. | |
1554 | It's not quite enough to say that the mul corresponds to line 2. The test should be checking that the mul looks correct after -pass-to-test runs, and it should explain why line 2 is the correct line for the mul to be on. |
This is looking really nice already! Some more suggestions inline --
docs/SourceLevelDebugging.rst | ||
---|---|---|
1499 | It looks like this is under-indented by 3 spaces? | |
1514 | Ditto, it looks like the line containing the 'alloca' is over-indented, and some of the other lines are under-indented by 3 spaces. | |
1546 | 'Instructions', 'Debug', and 'Location' don't need to be capitalized here. | |
1547 | Maybe better worded as: "to an instruction having a type that's incompatible with the source variable it describes" | |
1553 | The 'Fixing errors' section should be at a higher level. Please describe which APIs you'd use to fix each issue and why. You don't need to include code examples of how these APIs are used: assume that a developer reading this can find the relevant documentation and examples. Also, assume that readers will be able to locate problematic areas in the code. Missing debug location => Instruction::setDebugLoc, or possibly IRBuilder::setCurrentDebugLocation when using a Builder and the new location should be reused | |
1585 | Please always capitalize sentences and end them with periods. |
- Address inline comments.
In the debugify utility section I can't get the llvm code blocks
to print correctly. I can't spot any difference with the last one
in debugify in regression tests yet in the compiled html this first
two show like plain monospace while the last is colored correctly.
This looks really close, thanks! Phabricator is rendering all of the code blocks correctly -- there doesn't seem to be a formatting issue on this end.
docs/SourceLevelDebugging.rst | ||
---|---|---|
1468 | I don't know how I missed this earlier, but I think the title should be a little more specific. This section is more about 'testing optimizations with debug info', than it is about actually debugging optimized code. | |
1563 | 'my moving instructions' -> 'salvaging or deleting invalid debug values' | |
1634 | I still think we should add a caveat here. In order to write this type of regression test in a way that's not brittle, it's important to avoid hardcoding line/variable numbers in check lines (please explain why). In cases where that can't be avoided (say, if a test wouldn't be precise enough), it's a good practice to move the test into its own file. |
Thanks! LGTM, this seems like it's in great shape.
Could you wait a few days before committing this? Others may have further corrections/improvements to suggest.
Please change the patch title to match the title of the newly-added section. I have a few more minor suggestions inline, but overall this is ready to land.
docs/SourceLevelDebugging.rst | ||
---|---|---|
1591 | "append -check-debugify" -> "append -check-debugify -strip" | |
1604 | "first pass that applies synthetic DI" -> "-debugify pass" | |
1608 | Here's a way to rephrase the last sentence for extra clarity: 'Changes to this pass are not allowed to break existing tests.' | |
1617 | "-inst-combine" -> "-instcombine" |
I don't know how I missed this earlier, but I think the title should be a little more specific. This section is more about 'testing optimizations with debug info', than it is about actually debugging optimized code.