Page MenuHomePhabricator

[clangd] More complete fix for hover crashes on invalid record.
ClosedPublic

Authored by hokein on Jul 5 2020, 11:35 PM.

Details

Summary

We should not call getFieldOffset on invalid record decls.

Diff Detail

Event Timeline

hokein created this revision.Jul 5 2020, 11:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2020, 11:35 PM

thanks for doing this !

clang-tools-extra/clangd/Hover.cpp
667

i think we should bail out for invalid decls in here too. it won't crash but will provide spurious results. so i think an early exit for ND.isInvalidDecl() at the beginning of the function would be nice.

679–680

could you move this out of the if statement.

hokein updated this revision to Diff 275664.Jul 6 2020, 4:44 AM
hokein marked 2 inline comments as done.

address comment.

clang-tools-extra/clangd/Hover.cpp
679–680

I'm not sure whether Size and Offset should be be independent, IIUC your fix seems to change it as a side effect, this just preserves the old behavior.

kadircet added inline comments.Jul 6 2020, 7:51 AM
clang-tools-extra/clangd/Hover.cpp
679–680

it wasn't a side effect, it was intentional and stamped (and explicitly spelled out in the description)

kadircet accepted this revision.Jul 6 2020, 8:04 AM
This revision is now accepted and ready to land.Jul 6 2020, 8:04 AM
hokein marked an inline comment as done.Jul 6 2020, 8:07 AM
hokein added inline comments.
clang-tools-extra/clangd/Hover.cpp
679–680

ah, OK, changed it back.

hokein updated this revision to Diff 275718.Jul 6 2020, 8:08 AM

address comment.

This revision was automatically updated to reflect the committed changes.