Closed Bug 603346 Opened 9 years ago Closed 9 years ago

unreachable code in harfbuzz HeadlessArrayOf::sanitize and GenericArrayOf::sanitize

Categories

(Core :: Graphics, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED DUPLICATE of bug 603879

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(1 file)

668 template <typename Type>
669 struct HeadlessArrayOf
670 {
684   inline bool sanitize (hb_sanitize_context_t *c) {
686     if (unlikely (!sanitize_shallow (c))) return false;
691     return true;

this code is unreachable:
692     /* We do keep this code though to make sure the structs pointed
693      * to do have a simple sanitize(), ie. they do not reference
694      * other structs. */
695     unsigned int count = len ? len - 1 : 0;
696     Type *a = array;
697     for (unsigned int i = 0; i < count; i++)
698       if (unlikely (!a[i].sanitize (c)))
699         return false;
700     return true;
550 template <typename LenType, typename Type>
551 struct GenericArrayOf
552 {
573   inline bool sanitize (hb_sanitize_context_t *c) {
574     TRACE_SANITIZE ();
575     if (unlikely (!sanitize_shallow (c))) return false;
580     return true;

this code is unreachable:
581     /* We do keep this code though to make sure the structs pointed
582      * to do have a simple sanitize(), ie. they do not reference
583      * other structs. */
584     unsigned int count = len;
585     for (unsigned int i = 0; i < count; i++)
586       if (array[i].sanitize (c))
587         return false;
588     return true;
589   }
Summary: unreachable code in harfbuzz HeadlessArrayOf::sanitize → unreachable code in harfbuzz HeadlessArrayOf::sanitize and GenericArrayOf::sanitize
Attached patch remove itSplinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #482489 - Flags: review?(jfkthame)
It looks from the comments here as though Behdad wants to keep that code around, at least for the time being, even though it's obviously unreachable. Is there any reason we should worry about it?

(Actually, the dead code looks logically wrong in the GenericArrayOf case; I'd think it should be

    if (!array[i].sanitize(c))
      return false;

within the loop (the "!" is missing). So I suppose it's a good thing it doesn't get executed...)
People tend to assume code works. Conversely they sometimes will assume that an early return is accidental. As such keeping bad code in is a recipe for trouble.

I happened to read:
http://www.usenix.org/events/sec08/tech/slides/engler_slides.pdf
last night. I think you'll find that it argues against dead code. This isn't the only piece of buggy dead code I hit in my most recent round either.

Anyway, I don't own the code, you can do what you like.
Thanks Jonathan, fixed the missing negation.

So, I do need to have the compiler check that array[0].sanitize() exists.  I can of course do some other kind of unreachable code that the static-analyzers are less grumpy about, something like "return 0 ? array[0].sanitize() : true", but that doesn't change the bottom-line fact that I just want to make sure that function compiles without running it.
i'd expect Coverity to say that 'array[0].santitize()' is unreachable [=deadcode] in your example....
Comment on attachment 482489 [details] [diff] [review]
remove it

I think we should resolve this as WONTFIX, as I don't think there's any compelling reason for us to patch harfbuzz here.

If Behdad chooses to modify how this is handled upstream, we'll pick up those changes, but it's likely that some "dead" code will be deliberately kept around in some form.
Attachment #482489 - Flags: review?(jfkthame) → review-
Yeah.  I'm playing with it now to see if I can get a way that makes gcc's -Wunreachable-code happy.  No guarantees on coverity.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 603879
You need to log in before you can comment on or make changes to this bug.