Page MenuHomePhabricator

PostfixExpression: Use signed integers in IntegerNode
ClosedPublic

Authored by labath on Apr 30 2019, 6:54 AM.

Details

Summary

This is necessary to support parsing expressions like ".cfa -16 + ^", as
that format is used in breakpad STACK CFI expressions.

Since the PDB expressions use the same parser, this change will affect
them too, but I don't believe that should be a problem in practice. If
PDBs do contain the negative values, it's very likely that they are
intended to be parsed the same way, and if they don't, then it doesn't
matter.

In case that we do ever need to handle this differently, we can always
make the parser behavior customizable, or just use a different parser.

To make sure that the integer size is big enough for everyone, I switch
from using a (unsigned) 32-bit integer to a 64-bit (signed) one.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Apr 30 2019, 6:54 AM
amccarth accepted this revision.Apr 30 2019, 7:46 AM

This looks fine to me, and the rationale sounds right. I'll be curious to see if any other reviewers see a potential problem with this.

This revision is now accepted and ready to land.Apr 30 2019, 7:46 AM
clayborg added inline comments.Apr 30 2019, 2:29 PM
include/lldb/Symbol/PostfixExpression.h
97 ↗(On Diff #197320)

Do we want to try and use lldb_private::Scalar here? Then this could handle signed/unsigned ints, floats and anything else that comes along? Might need to be renamed "ScalarNode"? All of the type promotion stuff is already built into those classes.

labath marked an inline comment as done.May 2 2019, 1:59 AM
labath added inline comments.
include/lldb/Symbol/PostfixExpression.h
97 ↗(On Diff #197320)

I think that would be too generic for the current use case. For instance, that'd mean I'd have to figure out how to serialize the floats into a dwarf expression. I'm not sure if that's possible, but it doesn't seem terribly useful, as the purpose of these expressions is to compute addresses of stuff, and addresses are integral. If it turns out 64 bit signed int isn't enough, then I'd go for llvm::APSInt, but so far I don't see anything which would suggest that will ever be the case.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2019, 8:55 AM