[PATCH] D144565: dwp check overflow

[zhuna via Phabricator via llvm-commits]

zhuna8616 added a comment.

I removed the debug_str section checks because it seemed redundant.

Also made separate tests for dwarf v4 v5.



================
Comment at: llvm/lib/DWP/DWP.cpp:192
+  StringRef SectionName) {
+  std::string Msg = SectionName.str() + std::string(" Section Contribution Offset overflow 4G. Previous Offset ")
+    + std::to_string(PrevOffset) + std::string(", After overflow offset ") 
----------------
steven.zhang wrote:
> Do you format the changes ?
done


================
Comment at: llvm/lib/DWP/DWP.cpp:193
+  std::string Msg = SectionName.str() + std::string(" Section Contribution Offset overflow 4G. Previous Offset ")
+    + std::to_string(PrevOffset) + std::string(", After overflow offset ") 
+    + std::to_string(OverflowedOffset) + std::string(".");
----------------
steven.zhang wrote:
> I would prefer to use the StringRef to concat them, and then, convert base to std::string.
done


================
Comment at: llvm/lib/DWP/DWP.cpp:234
     C.setOffset(TypesOffset);
+    uint32_t OldOffset = TypesOffset;
     TypesOffset += C.getLength();
----------------
ayermolo wrote:
> Maybe a static_assert that sizeof(OldOffset) == sizeof(TypeOffset)?
done


================
Comment at: llvm/lib/DWP/DWP.cpp:238
+      Error Err (sectionOverflowErrorOrWarning(OldOffset, TypesOffset, "Types"));
+      if (Err) {
+        return Err;
----------------
dblaikie wrote:
> steven.zhang wrote:
> > Please remove the {} if there is only one statement in the if clause. The same rule applies in other changes.
> We'd also usually roll in the initialization to the if condition, and prefer `=` init over `()` init (avoids explicit conversions - so it's easier to read without worrying about any more "interesting" conversions happening):
> ```
> if (Error Err = sectionOverflowErrorOrWarning(...))
>   return Err;
> ``` 
done


================
Comment at: llvm/lib/DWP/DWP.cpp:688
       ContributionOffsets[Index] += CurEntry.Contributions[Index].getLength32();
+      if (ContributionOffsets[Index] < OldOffset) {
+        uint32_t SectionIndex = 0;
----------------
ayermolo wrote:
> OldOffset > ContributionOffsets[Index]
> Just to keep it consistent with other checks.
done


================
Comment at: llvm/lib/DWP/DWP.cpp:728-729
+          }
+            // return make_error<DWPError>(
+            //     "debug information section offset is greater than 4GB");
 
----------------
steven.zhang wrote:
> Remove this comments.
done


================
Comment at: llvm/test/tools/llvm-dwp/Inputs/overflow/debug_info.s:79
+.Lskel_string0:
+	.asciz	"/data00/home/zhuna.1024/gdb-10.1/hello" # string offset=0
+.Lskel_string1:
----------------
steven.zhang wrote:
> remove such kind of information
done


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144565/new/

https://reviews.llvm.org/D144565