This is an archive of the discontinued LLVM Phabricator instance.

Replace HashStringUsingDJB with llvm::djbHash
ClosedPublic

Authored by labath on Feb 21 2018, 3:24 PM.

Details

Summary

The llvm function is equivalent to this one. Where possible I tried to
replace const char* with llvm::StringRef to avoid extra strlen
computations. In most places, I was able to track the c string back to
the ConstString it was created from.

This also removes the unused ExportTable class.

Event Timeline

labath created this revision.Feb 21 2018, 3:24 PM
aprantl accepted this revision.Feb 21 2018, 3:59 PM
aprantl added a reviewer: clayborg.
aprantl added inline comments.
include/lldb/Core/MappedHash.h
156

Yeah this looks like it was a dead end dating back to 2011.

source/Target/ObjCLanguageRuntime.cpp
49

m_hash_to_isa_map.insert({llvm::djbHash(class_name), isa});

This revision is now accepted and ready to land.Feb 21 2018, 4:01 PM
davide accepted this revision.Feb 21 2018, 4:04 PM

Thanks.

We probably want to have a test to ensure this stays in sync with the hashes we generate for object files and dSYMs?

clayborg accepted this revision.Feb 22 2018, 7:41 AM

No need to add additional dSYM tests, because all dSYM tests will fail if the hashing isn't correct.

No need to add additional dSYM tests, because all dSYM tests will fail if the hashing isn't correct.

Maybe I misunderstand, but D42740 suggested that this never worked (or maybe it did accidentally, because we ended up in the right bucket) for unicode characters?

The only input we currently use this on is DWARF and I believe all DWARF strings are UTF8 so this should all work correctly, no?

The only input we currently use this on is DWARF and I believe all DWARF strings are UTF8 so this should all work correctly, no?

The hashing algorithms produced different hashes for characters with bit 7 set. That would apply for all UTF-8 characters that are outside of the 7-bit ascii range.

Jonas, do we have any tests in LLDB that try to lookup a variable with non-ASCII characters?

labath updated this revision to Diff 135568.Feb 22 2018, 5:06 PM

This adds a test which looks up a type with unicode characters. I have verified
this test does not pass when run against a pre-D42740 compiler.

This revision was automatically updated to reflect the committed changes.
amccarth added inline comments.
lldb/trunk/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py
13 ↗(On Diff #135658)

Oops! Looks like this Python file got checked in with a mixture of tabs and spaces.