This is an archive of the discontinued LLVM Phabricator instance.

Re-submit r289925 (Update .debug_line section version to match DWARF version)
ClosedPublic

Authored by kromanova on Sep 18 2017, 3:17 PM.

Details

Summary

This is one more attempt to re-land patch r289925 that was reverted by Nico a while ago (because it caused problem for Chromium's symbolizer).

Nico filed a bug PR# 31407, https://bugs.llvm.org/show_bug.cgi?id=31407, but unfortunately, the reproducer was not created for several months.
A couple of days ago Caroline confirmed that the problem was with the symbolizer, not with the patch itself. See all the details in PR# 31407.

So now I could re-submit the patch again.

I rebased my old patch (https://reviews.llvm.org/D16697) and made a few small changes. Namely, I updated debug_line table length/prologue length for some of newly added tests
and temporarily lowered the line table version to 2, when generation of DWARF V5 is requested (simply because line table V5 is not supported yet and a lot of newly added DWARF 5 tests started to fail).

Diff Detail

Repository
rL LLVM

Event Timeline

kromanova created this revision.Sep 18 2017, 3:17 PM
dblaikie added inline comments.Sep 20 2017, 9:41 AM
lib/MC/MCDwarf.cpp
282–286 ↗(On Diff #115731)

That seems weird - why not LineTableVersion 4? (why would going from DWARF 4 to DWARF 5 make the line table version go from 4 to 2?)

kromanova updated this revision to Diff 116058.Sep 20 2017, 1:01 PM

Thank you for the review, David! I have changed debug line version to V4, when DWARF V5 is requested (this is just a temporary measure until debug line V5 is supported).

probinson accepted this revision.Nov 27 2017, 4:25 PM

LGTM. Sorry, I hadn't realized nobody formally approved this yet! I'm starting to work on emitting the v5 line-table header so it would be nice to get this in.

This revision is now accepted and ready to land.Nov 27 2017, 4:25 PM
This revision was automatically updated to reflect the committed changes.
davide added a subscriber: davide.Dec 6 2017, 2:10 PM

I'm afraid this commit and the subsequent by @probinson broke large part of the debugger test suite.
I'll try updating all the tests to see if it's just as easy as that, but in case it won't I'll consider asking you to revert this commit.

davide added a comment.Dec 6 2017, 2:12 PM

More generally, DWARF changes should not impact the lldb test suite (apparently one of the bots, green dragon, failed, and now has been red for days).

davide added a comment.EditedDec 6 2017, 2:20 PM

This is the current status.

===================
Test Result Summary
===================
Test Methods:       2679
Reruns:                3
Success:            1626
Expected Failure:    106
Failure:             510
Error:                67
Exceptional Exit:      0
Unexpected Success:   44
Skip:                323
Timeout:               3
Expected Timeout:      0

I'm going to revert the two commits to bring the bot green again, then we can probably evaluate a strategy on how to tackle this.
@probinson/ @kromanova are you OK with this?

More generally, DWARF changes should not impact the lldb test suite (apparently one of the bots, green dragon, failed, and now has been red for days).

The issue is not the lldb test suite per se, but the lldb test suite as run on certain Apple bots and depending on certain system utilities.
The lldb tests that I ran on Linux were fine (ninja check-lldb check-lldb-lit, using the just-built clang as the test compiler).

The lldb bots did not send me a blame email, either. I followed up with the greendragon bot that was having the exact same problem with the debuginfo-tests. I was not aware of the lldb bot problem until today when somebody mentioned it on lldb-dev.

davide added a comment.Dec 6 2017, 2:26 PM

More generally, DWARF changes should not impact the lldb test suite (apparently one of the bots, green dragon, failed, and now has been red for days).

The issue is not the lldb test suite per se, but the lldb test suite as run on certain Apple bots and depending on certain system utilities.
The lldb tests that I ran on Linux were fine (ninja check-lldb check-lldb-lit, using the just-built clang as the test compiler).

This, FWIW, breaks also on my desktop (and probably and stock macOS configuration).
I understand it's not your fault, and I'll try to monitor fix the bots to send mails, but, in the meanwhile, I'd rather unblock this situation. Are you OK with temporarily reverting?

Let me see if I can find a way to force version 2 for Darwin only. I really don't want to revert again, it took months to get Chrome to fix their tools and reverting would block further work.

davide added a comment.Dec 6 2017, 2:36 PM

Thank you Paul. I'm OK with this. I don't want to bother you more than needed, probably I can try to take a look at how to implement myself.

davide added a comment.Dec 6 2017, 2:36 PM

(as I also have the means to check on the bots etc..)

Ah, thanks for the help Davide! Looks like you can do this:
if (context.getObjectFileInfo()->getTargetTriple().isOSDarwin()) LineTableVersion = 2;
and that should take care of it.

davide added a comment.Dec 6 2017, 3:00 PM

No worries, happy to get this solved. FWIW, there was a configuration problem and this why you didn't receive the mail.
This was fixed so you should be able to receive blame mails in future (please let me know if you don't).