[PATCH] Add DWARFFormValue::getAsSignedConstant().

Frederic Riss friss at apple.com
Mon Oct 6 14:53:24 PDT 2014


================
Comment at: lib/DebugInfo/DWARFFormValue.cpp:554
@@ +553,3 @@
+  if ((!isFormClass(FC_Constant) && !isFormClass(FC_Flag))
+      || Form == DW_FORM_udata)
+    return None;
----------------
dblaikie wrote:
> friss wrote:
> > dblaikie wrote:
> > > hmm - why is udata considered "not signed". It's specifically saying that the value is positive, so should be returned successfully that way, no?
> > > 
> > > Arguable, I suppose - but I wouldn't put it past implementations to return positive data in _udata to reduce the number of bytes needed to encode it.
> > Right. This check wasn't there at first and I added it to prevent issues with values > LONG_MAX being returned as negative numbers. Given how we'll use the method this isn't really an issue.
> > 
> > Note that getAsUnsignedConstant rejects FORM_sdata. It has more reasons to do so, but along the same line of thought, it should maybe return a value if he sdata is in fact positive? 
> I believe the right thing would be for the value to be printed as signed/unsigned based on udata/sdata - or based on context (form kind, etc) if it's dataN. Not sure how convenient that is to implement.
But here the question isn't even about printing, but only about accepting udata in getAsSignedConstant() and sdata in getAsUnsignedConstant(). Would you prefer the values to be accepted if they fit into the output range and return None otherwise?

================
Comment at: unittests/DebugInfo/DWARFFormValueTest.cpp:10
@@ -9,2 +9,3 @@
 
+#include "../lib/DebugInfo/DWARFContext.h"
 #include "llvm/DebugInfo/DWARFFormValue.h"
----------------
samsonov wrote:
> Unittests may only include headers from include/llvm (i.e., public headers). If we want to add unittests that instantiate DWARFContext, we should make corresponding header public.
I was actually wondering about this, but I found one prior example of including ../lib/. I'm not sure what the best way forward is for this, if we make the Context public, then everything gets public. Not that I'm opposed to that, it would make some of my planned work easier, and I might propose that in the near future.

What would people think about making the whole DWARF stuff a pubilc interface to libDebugInfo ? (Note that right now this is just to get some testing for this very small feature  added just to be able to print AT_lower_bound as a signed decimal... I was already embarrassed by the amount of Dummy* instantiations to get my small test running, but getting the whole interface made public just for that seems a bit too much, doesn't it?)

http://reviews.llvm.org/D5630






More information about the llvm-commits mailing list