[PATCH] D144565: dwp check overflow

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:475
+      }
+      ContributionOffsets[Index] = NewOffset.getLimitedValue();
     }
----------------
zhuna8616 wrote:
> ayermolo wrote:
> > zhuna8616 wrote:
> > > zhuna8616 wrote:
> > > > ayermolo wrote:
> > > > > Please make sure this doesn't break https://reviews.llvm.org/D137882
> > > > > It relies on overflow behavior.
> > > > When nothing overflows, getLimitedValue simply returns the raw data of this APInt. Since there is an overflow check, this is synonyms to something like getValue.
> > > I tried it on your test case in https://reviews.llvm.org/D137882 and passed.
> > > {F26615950}
> > Sorry wasn't clear. That test doesn't use llvm-dwp, I just pointed to the diff to point out that there is a code in llvm that relies on overflow behavior to reconstruct cu-index. 
> > There is a downstream option for us that changes error for previous check into warning. Forgot I didn't upstream it.
> > https://github.com/llvm/llvm-project/blob/265ea1c7459da95b7a1d30718ac9f970a3ecc8d1/llvm/lib/DWP/DWP.cpp#L672
> > (which probably should be removed if check is added here).
> > Actually now that all the changes have landed in llvm and lldb, maybe there should be an option to turn it into an warning? At least for .debug_info part.
> This patch is meant to backport llvm/11.x version, where no overflow check exists. The latest implementation checks overflow in debug_info section only. Am I supposed to turn the error introduced in this patch to a warning?
No. I am proposing adding an option that when it's turned on this becomes a warning, and overflow behavior is preserved.
Also since this patch also checks .debug_info overflow, the other check can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144565