[PATCH] Add DWARFFormValue::getAsSignedConstant().

David Blaikie dblaikie at gmail.com
Mon Oct 6 13:36:06 PDT 2014


================
Comment at: lib/DebugInfo/DWARFFormValue.cpp:554
@@ +553,3 @@
+  if ((!isFormClass(FC_Constant) && !isFormClass(FC_Flag))
+      || Form == DW_FORM_udata)
+    return None;
----------------
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.

================
Comment at: unittests/DebugInfo/DWARFFormValueTest.cpp:103
@@ +102,3 @@
+TEST(DWARFFormValue, SignedConstantForms) {
+  DWARFFormValue data1 = createFromValue(DW_FORM_data1, 0xFF);
+  DWARFFormValue data2 = createFromValue(DW_FORM_data2, 0xFFFF);
----------------
friss wrote:
> dblaikie wrote:
> > Does this need tests for any other cases? (these tests would all pass with "getAsSignedConstant()" implemented as a single "return -1")
> My main concern was to check that I got the sign extension right for all the FORM_data*. Some other cases can of course be added. Do you have any specific limit tests in mind that  would be interesting, or just adding a few positive and negative values to the test would do it for you?
Yeah, I'd probably just try testing a few other values (potentially replacing the 4 values tested here with more variety - or at least some values in the middle of negatives/positive values, etc)... but maybe it doesn't matter. Up to you.

http://reviews.llvm.org/D5630






More information about the llvm-commits mailing list