Page MenuHomePhabricator

Fix crash after looking up dwo_id=0 in CU index.
ClosedPublic

Authored by jgorbe on Tue, Nov 17, 5:32 PM.

Details

Summary

In the current state, if getFromHash(0) is called and there's no CU
with dwo_id=0, the lookup will stop at an empty slot, then the check
Rows[H].getSignature() != S won't fail, because the empty slot has
a 0 in the signature field, and a pointer to the empty slot will be
returned.

This patch fixes this by using the index field in the hash entry to
check for empty slots: signature = 0 can match a valid hash but
according to the spec the index for an occupied slot will always be
non-zero.

Diff Detail

Event Timeline

jgorbe created this revision.Tue, Nov 17, 5:32 PM
Herald added a project: Restricted Project. · View Herald Transcript
jgorbe requested review of this revision.Tue, Nov 17, 5:32 PM
dblaikie added inline comments.Tue, Nov 17, 5:54 PM
llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
6–7

Checking for "anything other than crashing" is a bit loose - could you check for the specific failure you expect when the dwo_id 0 can't be found? (I guess the symbolizer won't include some details in the symbolized stack trace, for instance)

MaskRay added inline comments.Tue, Nov 17, 5:56 PM
llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
6

If llvm-symbolizer segfaults, I think it is not guaranteed that "Stack dump" will be printed.
Since a segfaulting process has a non-zero exit code, simply writing RUN: llvm-symbolizer --obj=%t --dwp=%t.dwp 0x0 performs the check.

This is probably sufficient for a regression test. However, if you have other interesting output to check, consider adding them to make the test better.

jgorbe updated this revision to Diff 305947.Tue, Nov 17, 6:05 PM

Changed test expectation from "not a stack dump" to the actual output from llvm-symbolizer for the test case.

jgorbe marked 2 inline comments as done.Tue, Nov 17, 6:08 PM

Thanks for the comments!

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
6

Changed to expect the output of the fixed symbolizer as suggested by the other comment. I'm not sure what option is better, this test case is basically a regression test.

6–7

The output is extremely uninteresting, but done.

dblaikie accepted this revision.Tue, Nov 17, 6:52 PM

Looks good, thanks!

This revision is now accepted and ready to land.Tue, Nov 17, 6:52 PM

then the check Rows[H].getSignature() != S won't fail

The condition fails.

MaskRay added inline comments.Tue, Nov 17, 9:23 PM
llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
4

Since MAIN and DWP are mutual exclusive, you can just define one macro and use

.ifdef
...
.else
...
.endif

I usually write x86_64-pc-linux as x86_64 to represent that this is a generic ELF behavior, rather than a Linux specific one.

94

For version 2, the field should be .long 2

(The difference matters if this is big-endian)

MaskRay added inline comments.Tue, Nov 17, 9:38 PM
llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
49

A\I is undefined. Consider defining the symbol to be more realistic.

106

The two loops can be merged.

jhenderson added inline comments.Wed, Nov 18, 12:32 AM
llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
2

I'd find it useful if there was a top-level comment in this test explaining the purpose of this test.

8

A number of our newer binary utility tests use the pattern of ## for comments, to clearly distinguish them from lit and FileCheck lines. Could you do that in this test too?

labath added inline comments.Wed, Nov 18, 1:23 AM
llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
94

This is kind of my fault, as this source is adapted from one of my tests. DWP version 2 (unofficial fission proposal) does indeed use a single long field, but then the official standardized format (version 5) uses two short fields -- and I have mixed the two up.

106

Well, if it's going to have just one iteration, then I guess the loops can just be deleted altogether.

jgorbe updated this revision to Diff 306176.Wed, Nov 18, 11:38 AM
jgorbe marked 7 inline comments as done.

Addressed reviewer comments.

jgorbe marked 3 inline comments as done.Wed, Nov 18, 11:40 AM

Updated diff addressing the new round of feedback.

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
2

Done.

4

Removed the DWP macro and folded all into the same .ifdef/.else/.endif.

Changed triple from x86_64-pc-linux to x86-64

8

Sure! Done.

49

Good catch! Went a bit too far trimming the lldb test this is based on :)

94

Fixed.

106

Removed all the loops for the DWP case as they had only one iteration.

MaskRay accepted this revision.Wed, Nov 18, 11:42 AM

LGTM.

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

jgorbe marked 2 inline comments as done.Wed, Nov 18, 12:26 PM

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

Yeah, I could have expressed this better in the summary. What I meant with "the check won't fail" was that the check Rows[H].getSignature() != S is not enough to cause the lookup to fail and return a nullptr (as it should). I'll fix it in the actual commit message.

labath accepted this revision.Thu, Nov 19, 12:30 AM

Not looked at the whole thing in depth, but my comments have been addressed, so happy for this to land from my point of view, if others are happy.

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
49

It's probably not applicable here, but FYI, yaml2obj now has some DWARF support which allows you to craft sections using YAML rather than assembly, without needing to specify all details. Take a look at some of the tests that are using it for examples of how you might use it.

Thanks everyone for the reviews! I'm committing this now.

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s
49

Thanks for the tip! I'll have a look so I can use it in future tests :)

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

Yeah, I could have expressed this better in the summary. What I meant with "the check won't fail" was that the check Rows[H].getSignature() != S is not enough to cause the lookup to fail and return a nullptr (as it should). I'll fix it in the actual commit message.

You can edit the summary to be synchronized with the commit. If you upload differentials with arc diff --verbatim (--verbatim allows you to modify the summary while uploading a new patch).

This revision was automatically updated to reflect the committed changes.