Page MenuHomePhabricator

Fix BucketT Handling in DenseMap.
Changes PlannedPublic

Authored by varno on Feb 6 2017, 3:26 PM.

Details

Summary

DenseMap did not copy complete buckets when moving, growing or copying. This patch makes that happen.

Event Timeline

dberris requested changes to this revision.Feb 6 2017, 3:56 PM

You need to add unit tests to show what the bug is, and how this change is fixing it. That should be added as a test case in the existing unit test for DenseMap. Right now I'm inclined to reject this change given the issue raised inline.

include/llvm/ADT/DenseMap.h
110

This smells funny -- first you destroy the object, then call a function on the destroyed object. I'm positive you are running into undefined behaviour here. To fix this, you should attempt to re-initialize the object pointed to through P using placement new.

This revision now requires changes to proceed.Feb 6 2017, 3:56 PM
varno added inline comments.Feb 6 2017, 5:27 PM
include/llvm/ADT/DenseMap.h
110

This is no more smelly than the initial problems. if you look at the source code, the BucketTs are not initialized anywhere. There is never a constructor called on them. This is no different to how they are initialized initially. See line 326, in the file.

I am just making it less bad. Really we need to initialize the buckets correctly, however this requires more work than I can spare.

varno updated this revision to Diff 87346.Feb 6 2017, 5:57 PM
varno edited edge metadata.
  • clang-format
dberlin requested changes to this revision.Feb 6 2017, 6:04 PM
dberlin added a subscriber: dberlin.

As Dean said,

  1. You need a very clear testcase demonstrating the issue you are trying to solve.
  2. A clear description how the change solves the problem.

What it looks like now is "a change people are having trouble understanding the reasoning for" and "an argument that world is broken right now".

That is leaving people confused.

Saying "I am just making it less bad. Really we need to initialize the buckets correctly, however this requires more work than I can spare." doesn't help.

It's okay if you don't have time to fix it right.
File a bug, with a reproducible testcase demonstrating what's wrong, and someone who does have time will probably take a stab at it.

In the meantime, we probably don't want to hack up one of the most used datastructures in LLVM with an incompletely solution to a problem people don't understand yet.

This revision now requires changes to proceed.Feb 6 2017, 6:04 PM
varno planned changes to this revision.Feb 6 2017, 7:22 PM
varno added a comment.Feb 8 2017, 5:17 PM

I will work on fleshing out a bug report after I finish my internship on my own time.

dberris resigned from this revision.Aug 29 2017, 11:57 PM