[PATCH] D28456: DebugInfo: support for DW_FORM_implicit_const

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

A few things to cleanup, but feel free to commit after that. If there are any wrinkles with my suggestions do let me know.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:34-38
     /// contained in or the value size varies and must be decoded from the
     /// debug information in order to determine its size.
     Optional<uint8_t> ByteSize;
+    /// Optional value (for DW_FORM_implicit_const).
+    Optional<int64_t> Value;
----------------
vleschuk wrote:
> dblaikie wrote:
> > These are mutually exclusive, right? (if there's a value, the byte size is zero) - could we coalesce them in some way?
> We could use simply int64_t and bool field, smth like:
> 
> ```
> int64_t ByteSizeOrValue;
> bool IsImplicitConst;
> ```
> But will it gain us much?
Perhaps doesn't matter - was just wondering about the size of ATtributeSpec, but it's probably not too important and we can easily optimize it later if it turns out to be important.


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:60-61
+      Optional<int64_t> V;
+      bool IsImplicitConst = false;
+      if (F == DW_FORM_implicit_const) {
+        IsImplicitConst = true;
----------------
I'd probably roll this condition into the variable:

  bool IsImplicitConst = F == DW_FORM_implicit_const;
  if (IsImplicitConst)
    V = Data.getSLEB128(OffsetPtr);
  else if (auto Size = DWARFFormValue::getFixedByteSize(F))
    V = *Size;
  Attributes.push_back(AttributeSpec(A, F, V));
  if (IsImplicitConst)
    continue;

But also - why does IsImplicitConst produce the need for this branch here, when flag_present did not? I suspect it's probably better to leave the fixed byte size calculation in and let that handle the implicit const rather than adding a special case.

Ah, I see - you're passing the size-or-implicit-const into the AttributeSpec ctor. It might be more legible if it used a named ctor or the like to differentiate the parameter. How about something like this:

  auto FixedFormByteSize = DWARFFormValue::getFixedByteSize(F);
  AttributeSpecs.push_back(F == DW_FORM_implicit_const
      ? AttributeSpec::makeImplicitConst(A, Data.getSLEB128(OffsetPtr))
      : AttributeSpec(A, F, FixedFormByteSize));
  // If this abbreviation still has a fixed byte size, ...




================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:143
       OS << format("DW_FORM_Unknown_%x", Spec.Form);
+    if (Spec.isImplicitConst() && Spec.ByteSizeOrValue)
+      OS << '\t' << Spec.ByteSizeOrValue.getValue();
----------------
An implicit const should always have a value, right? Perhaps this should `assert(Spec.ByteSizeOrValue)` (or access the value unconditionally without an assert - the Optional will assert if there's no value)


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:144
+    if (Spec.isImplicitConst() && Spec.ByteSizeOrValue)
+      OS << '\t' << Spec.ByteSizeOrValue.getValue();
     OS << '\n';
----------------
Using the dereference operator can be a bit less verbose than using 'getValue()':

  ... << *Spec.ByteSizeOrValue;


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:177
+      if (Spec.isImplicitConst()) {
+        if (Spec.ByteSizeOrValue)
+          FormValue.setSValue(Spec.ByteSizeOrValue.getValue());
----------------
As above, I think it should be the right thing to assert(Spec.ByteSizeOrValue) or assume it's non-empty without an assertion, even.


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:178
+        if (Spec.ByteSizeOrValue)
+          FormValue.setSValue(Spec.ByteSizeOrValue.getValue());
+        return FormValue;
----------------
As above, probably use Optional's operator*


https://reviews.llvm.org/D28456