This is an archive of the discontinued LLVM Phabricator instance.

[NFC][DebugInfo] Refactor address advancing operations to share code
ClosedPublic

Authored by jhenderson on Feb 26 2020, 8:40 AM.

Details

Summary

On its own, this change won't achieve much, but it provides a good place to do extra error checks that I plan on adding in D43470, D74819 and D75189. See the full patch series for context.

Diff Detail

Event Timeline

jhenderson created this revision.Feb 26 2020, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2020, 8:40 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jhenderson edited the summary of this revision. (Show Details)Feb 27 2020, 2:48 AM

This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the LineTable::parse function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the parse function somehow, which is getting a bit unwieldy...

This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the LineTable::parse function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the parse function somehow, which is getting a bit unwieldy...

Yeah, I and a colleague who I discussed this with did have similar concerns, although we hadn't come up with a better solution yet. I did consider making these lambdas, although I'm not a massive fan of having big lambdas (no specific reason for that, just my personal feeling). There is a ParsingState struct already being passed around. Perhaps some of the local variables could be put in that (i.e. the error handler, the warning booleans and the table offset, perhaps even the opcode offset)? That would reduce advanceAddr's parameter count to 4 or 5 from 7, and getOperationAdvance from 8 to 6 or 5, I believe.

This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the LineTable::parse function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the parse function somehow, which is getting a bit unwieldy...

Yeah, I and a colleague who I discussed this with did have similar concerns, although we hadn't come up with a better solution yet. I did consider making these lambdas, although I'm not a massive fan of having big lambdas (no specific reason for that, just my personal feeling).

I don't particularly like care for big lambdas either. If we really wanted to we could keep this function, and create a lambda just to "fix" some parameters. However, I like the next idea better.

There is a ParsingState struct already being passed around. Perhaps some of the local variables could be put in that (i.e. the error handler, the warning booleans and the table offset, perhaps even the opcode offset)? That would reduce advanceAddr's parameter count to 4 or 5 from 7, and getOperationAdvance from 8 to 6 or 5, I believe.

Yes, the ParsingState is exactly what I was looking for. I think that adding these locals there is a great idea.

But I also like my idea of making advanceAddr a member function of this struct. I think that would be similar to the existing appendRowToMatrix. Just like the implementation of DW_LNS_copy consists of calling that method, DW_LNS_advance_pc could be implemented by calling State.advanceAddr. (The function could still return the offset which it used to increment the address to enable the verbose dump.)

jhenderson updated this revision to Diff 247602.Mar 2 2020, 4:39 AM

Rebase and further refactor things to reduce number of required arguments for the new functions in subsequent changes.

labath added a comment.Mar 3 2020, 2:32 AM

I think this looks a lot better, but someone else ought to give the final approval here..

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
380

calculated

jhenderson updated this revision to Diff 247868.Mar 3 2020, 5:40 AM

Fix grammar.

jhenderson marked an inline comment as done.Mar 3 2020, 5:40 AM
probinson accepted this revision.Mar 3 2020, 9:03 AM

LGTM

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
917

It seems like there ought to be a symbol for opcode 255, but you inherited the code so I won't insist.

This revision is now accepted and ready to land.Mar 3 2020, 9:03 AM
jhenderson marked an inline comment as done.Mar 4 2020, 3:00 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
917

I can add that in a separate change if you'd like, although I'm not entirely sure what I'd label it as, since the DWARF spec (as quoted by the comment above) simply calls it the "special opcode 255".

One final question and reiterate LGTM.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
917

Not necessary.
We do assert that maximum_operations_per_instruction is 1, right? The advance operations are not factoring that in, but if it's always 1 then the simplified operations are correct.

probinson added inline comments.Mar 4 2020, 8:48 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
917

Duh, that's D74819. Never mind, all good here.

jhenderson updated this revision to Diff 248464.Mar 5 2020, 6:25 AM
jhenderson marked 2 inline comments as done.

Get rid of getOperationAdvance function, as it was only being used in one place, so was unnecessarily complicated.

I agree this looks cleaner.

This revision was automatically updated to reflect the committed changes.