Remove dead code

Remove dead code

[Mark Mentovai]

http://breakpad.appspot.com/502003/diff/1/src/common/dwarf/dwarf2diehandler.h
File src/common/dwarf/dwarf2diehandler.h (right):

http://breakpad.appspot.com/502003/diff/1/src/common/dwarf/dwarf2diehandler.h...
src/common/dwarf/dwarf2diehandler.h:242: // it is; and ATTRS is the list of
attributes the DIE will have, and
If you’re going to do this, you’ve got to revise the documentation too.

[jimb]

I think Mark should have the final say on this one, but for what it's worth, I
don't think this is a good change.

Rafael previously submitted a less expansive version of this patch, which I
declined to approve, here: https://breakpad.appspot.com/477002/  We had a longer
discussion about the change on a Mozilla IRC channel at the time.

Rafael is correct that the handlers we use with dwarf2reader::CompilationUnit
and dwarf2reader::DIEHandler do not use the attribute lists.

However, the job of dwarf2reader is to parse and present DWARF DIEs. The code
this patch would remove allows Dwarf2Handler or DIEHandler instances to decide
whether or not they're interested in a given DIE before bothering to parse the
DIE's attributes' values. The structure of DWARF means that the parser must
construct the attribute lists anyway. Since we have them at hand, simply passing
them through to the handlers requires passing a pointer argument along, not a
significant cost.

As it happens, our handlers don't use that information. But tailoring the
parser's API to the incidental characteristics of the handlers with which we
happen to use it today seems like a mistake, limiting the producer based on the
needs of one consumer.

At Mozilla we are currently experimenting with adapting Breakpad's DWARF CFI
readers for in-process use on live stacks. To improve performance, we have
replaced the components between the parser and the stack walker: we have written
new handlers and resolvers, so that we can store and cache the CFI data in a way
better suited to live use. In doing so, it was helpful that the parsing and
stackwalking interfaces were not particularly concerned with who was receiving
or providing the data.

Certainly, in a world with version control, we can always get it back if we need
it. But since it's not a performance hit, nor a real complexity hit, I don't
understand why it's worth removing.

[rafael.espindola]

> Rafael previously submitted a less expansive version of this patch, which I
> declined to approve, here: https://breakpad.appspot.com/477002/  We had a
longer
> discussion about the change on a Mozilla IRC channel at the time.

And I failed to see any sense in your arguments, hence the this patch.

> Rafael is correct that the handlers we use with dwarf2reader::CompilationUnit
> and dwarf2reader::DIEHandler do not use the attribute lists.

And that is all there is to it. The code is dead (and rotting) and should be
removed. Note things like

AttributeList child2_attribute_list;	
   child1_attribute_list.push_back(make_pair((DwarfAttribute) 0xf22df14c,	
                                             (DwarfForm) 0x20676e7d));

which clearly "wanted" to use child2, but since the code is dead it makes no
difference, other than being incorrect documentation that survives the compiler.

> However, the job of dwarf2reader is to parse and present DWARF DIEs. The code
> this patch would remove allows Dwarf2Handler or DIEHandler instances to decide
> whether or not they're interested in a given DIE before bothering to parse the
> DIE's attributes' values. The structure of DWARF means that the parser must
> construct the attribute lists anyway. Since we have them at hand, simply
passing
> them through to the handlers requires passing a pointer argument along, not a
> significant cost.
>

There is a lot more things that the caller can always know. The first 10 prime
numbers for example. Should I pass them along instead? They would have an even
lower cost to pass.

> As it happens, our handlers don't use that information. But tailoring the
> parser's API to the incidental characteristics of the handlers with which we
> happen to use it today seems like a mistake, limiting the producer based on
the
> needs of one consumer.

The code should be as simple as possible. The fact that the attributes are not
needed allows for simplifications.
 
The code style doesn't say "no dead code". The closest thing is "unused named
parameters" being commented, but not that example is one where we *must* have
the parameter because of how virtual functions work.

Not having dead code is just common sense. It is like saying that code must
follow the system ABI.

> Certainly, in a world with version control, we can always get it back if we
need
> it. But since it's not a performance hit, nor a real complexity hit, I don't
> understand why it's worth removing.

It is dead code. Its worth is at most 0. IMHO it is < 0 right now given that it
is both dead and incorrect.

[jimb]

On 2012/12/08 13:50:27, rafael.espindola wrote:
> The code is dead (and rotting) and should be
> removed. Note things like
> 
> AttributeList child2_attribute_list;	
>    child1_attribute_list.push_back(make_pair((DwarfAttribute) 0xf22df14c,	
>                                              (DwarfForm) 0x20676e7d));
> 
> which clearly "wanted" to use child2, but since the code is dead it makes no
> difference, other than being incorrect documentation that survives the
compiler.

This is misleading. That test has a minor bug; when fixed, it passes. Both
CompilationUnit and DIEHandler correctly pass the attribute lists to the
handler. The code has not bitrotted.

[Mark Mentovai]

I dunno, guys, I can see both sides of this one. In perfect isolation, this
would be dead code, and we’d just nuke it. But in the real world, it sounds like
this code is either the basis for other projects’ DWARF readers, or is based on
other projects’ DWARF readers, or both. This code may not be dead for the
purposes of those other projects, which might care about DIE attribute lists. In
that case, keeping the various implementations on the same page (or even
unifying them) is a very desirable property.

Which of these are we closer to? Is the DWARF reader in Breakpad to be evaluated
in isolation, severing ties to whatever it’s based on, and not expected to serve
as the basis for other projects’ DWARF readers? Or is this code active in other
projects, perhaps even to the point that it might be factored out into its own
fully-fledged project that Breakpad and those other projects could depend on?

[jimb]

Right, the central disagreement is whether or not the potential for re-use
should be taken into consideration.

I don't know of any other projects that use this DWARF parser. It does seem to
have been imported from somewhere else originally, but I don't know where.

I mentioned that Mozilla is re-using the CFI parser and the stack walkers for
our profiler. I suspect we will need the line info reader, and thus the DIE
parser, for that as well, once we get to the point of trying to translate the
addresses recovered by the unwinder to source positions.

DWARF is a pain to parse. I think it's of general value to the public to have
code out there that is well-exercised, well-tested, and offers a reasonable
interface.

We're talking about pretty trivial code, here. I don't see the harm in leaving
it there, and I see a potential benefit in doing so.

[rafael.espindola]

On 10 December 2012 15:59,  <mark@chromium.org> wrote:
> I dunno, guys, I can see both sides of this one. In perfect isolation,
> this would be dead code, and we’d just nuke it. But in the real world,
> it sounds like this code is either the basis for other projects’ DWARF
> readers, or is based on other projects’ DWARF readers, or both. This
> code may not be dead for the purposes of those other projects, which
> might care about DIE attribute lists. In that case, keeping the various
> implementations on the same page (or even unifying them) is a very
> desirable property.

We can't possibly anticipate all users. Why is the attribute list
special? Why not also pass the offset in the file? Or the 10 bytes the
precede it?

If a use shows up that needs more information, we can *then* discuss
what is the best way to make that information available. In
particular, I don't see how using breakpad online (i.e. in process)
would change this, but if I am wrong and a use does show up, I would
be more than happy to add *live* code.

Right now this code is a fairly painful burden. I found it while
working on r1062. Seeing the attributes being passed to
FindChildHandler I had to figure out if I needed to change them in the
test. To do so I had to go look at every implementation just to
noticed that they were all unused.

If for some higher reason that mere mortals cannot understand we
really must keep dead (.i.e.: Fully redundant) code, we should at
least be more kind to other mere mortals:

* Comment the unused arguments as required by the google style guide.
* Add a comment to all declarations saying that:
  1) The argument is dead, but for some higher reason it cannot be removed.
  2) Despite the fact that you cannot make a test pass or fail by
changing the contents of attr because of 1, there is somehow a
"correct" list to pass. Send to it jimb and he can figure the list
out.

Cheers,
Rafael

[Mark Mentovai]

Thanks for arguing this so vigorously, guys. Since there doesn’t seem to be a
present need to share this code with anyone else or adapt Breakpad to use anyone
else’s implementation, I think that the more rational option is to drop the code
for the time being. If we ever need it again in the future, we can resurrect it
from change history.

Jim, if there’s a bug in the test that you want to fix before we nuke the code
so that if we ever need to bring it to life, we can bring back something that we
think to be working, then that’s fine with me.

Otherwise, LGTM as-is.

[jimb]

On 2012/12/12 01:32:11, Mark Mentovai wrote:
> Thanks for arguing this so vigorously, guys. Since there doesn’t seem to be a
> present need to share this code with anyone else or adapt Breakpad to use
anyone
> else’s implementation, I think that the more rational option is to drop the
code
> for the time being. If we ever need it again in the future, we can resurrect
it
> from change history.
> 
> Jim, if there’s a bug in the test that you want to fix before we nuke the code
> so that if we ever need to bring it to life, we can bring back something that
we
> think to be working, then that’s fine with me.

I don't think it's pressing.

[Mark Mentovai]

Committed r1091. Thanks for the patch and the discussion.

[jimb]

On 2012/12/12 04:03:22, Mark Mentovai wrote:
> Committed r1091. Thanks for the patch and the discussion.

Sure thing! Thanks for your patience.