Summary: | StringHasher functions require alignment that call sites do not all guarantee | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
Component: | Web Template Framework | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, gyuyoung.kim, ojan.autocc, philn, rakuco, tmpsantos, webkit-ews, webkit.review.bot, xan.lopez, zan | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Darin Adler
2013-02-18 18:10:19 PST
Created attachment 188973 [details]
Patch
Comment on attachment 188973 [details]
Patch
Oops, looks like I uploaded something that doesn’t build.
Comment on attachment 188973 [details] Patch Attachment 188973 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16613441 Created attachment 188974 [details]
Patch
Comment on attachment 188974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188974&action=review The new methods addCharactersAssumingAligned() are public. Wouldn't it be better to have them private until they are needed to be public by client code? Exposing those low level functions increases the risk of misuses like the one you fix with this patch. If you keep the new methods public, it would be nice to have new test coverage for them. > Source/WTF/wtf/StringHasher.h:77 > - addCharactersToHash(m_pendingCharacter, Converter(*data++)); > - m_hasPendingCharacter = false; > - --length; > + addCharactersInternal(m_pendingCharacter, a); > + m_pendingCharacter = b; > + return; It would be nice to get rid of addCharactersInternal to simplify the class. Is there really any extra cost to?: m_hasPendingCharacter = false; addCharactersInternal(m_pendingCharacter, a); m_pendingCharacter = b; m_hasPendingCharacter = true; (In reply to comment #5) > The new methods addCharactersAssumingAligned() are public. Wouldn't it be better to have them private until they are needed to be public by client code? I suppose they could be private; my intention is that they are part of the public interface. > Exposing those low level functions increases the risk of misuses like the one you fix with this patch. Sure, but given their names it seems unlikely they’d be misused. The issue before was the hidden precondition that was not part of the name. I don’t think Geoff would have gotten it wrong if it was part of the name. > If you keep the new methods public, it would be nice to have new test coverage for them. That’s a good point. Whether public or not it would be good to have tests for the functions in this class. I don’t know exactly how to do unit tests; I’m sure it’s easy but I don’t know where to put them. > It would be nice to get rid of addCharactersInternal to simplify the class. Will do. Created attachment 191122 [details]
Patch
The patch was actually already reviewed by Ben Poulain. This new version makes most of the changes he suggested, except for the “make more private” one, which we could still do. And I added lots of tests. Comment on attachment 191122 [details]
Patch
Thanks for doing all the tests!
My only comment is it would be useful to have a comments for each group of test.
E.g.:
// addCharactersAssumingAligned with zero characters should produce an emptyStringHash.
// addCharactersAssumingAligned with \0.
// addCharactersAssumingAligned with 5 characters at once.
etc.
Otherwise it takes some reading to find what is tested in each paragraph.
Committed r144552: <http://trac.webkit.org/changeset/144552> (In reply to comment #10) > Committed r144552: <http://trac.webkit.org/changeset/144552> This test is failing on the EFL Debug bots from the moment it was introduced (although it is passing on the Release bots): /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:173: Failure Value of: hasher.hash() Actual: 2754808611 Expected: testBHash5 Which is: 2032145993 [ FAILED ] WTF.StringHasher_addCharacters (0 ms) [ RUN ] WTF.StringHasher_addCharactersAssumingAligned /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:328: Failure Value of: hasher.hash() Actual: 2754808611 Expected: testBHash5 Which is: 2032145993 [ FAILED ] WTF.StringHasher_addCharactersAssumingAligned (0 ms http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/10028/steps/API%20tests/logs/stdio I see the problem. I forgot to null-terminate testBUChars! Geoff Garen fixed the failing tests problem in r145412. |