[PATCH] D17733: [ELF] Fix reading of PC values of FDEs

[Rui Ueyama via llvm-commits]

I think I'm not familiarized to the idea of using lambda to eliminate
break's from switches. Not sure if I'll adopt that, but for now, I'd go
with the plain C-like approach.

On Tue, Mar 1, 2016 at 1:38 PM, Rafael Espíndola <rafael.espindola at gmail.com
> wrote:

> Small preference for a lambda, but LGTM without it too if Rui prefers
> it that way.
>
> Cheers,
> Rafael
>
>
> On 1 March 2016 at 16:30, Simon Atanasyan <simon at atanasyan.com> wrote:
> > atanasyan added inline comments.
> >
> > ================
> > Comment at: ELF/OutputSections.cpp:669-670
> > @@ -668,16 +668,4 @@
> >  typename EhFrameHeader<ELFT>::uintX_t
> >  EhFrameHeader<ELFT>::getFdePc(uintX_t EhVA, const FdeData &F) {
> > -  const endianness E = ELFT::TargetEndianness;
> > -  assert((F.Enc & 0xF0) != DW_EH_PE_datarel);
> > -
> > -  uintX_t FdeOff = EhVA + F.Off + 8;
> > -  switch (F.Enc & 0xF) {
> > -  case DW_EH_PE_udata2:
> > -  case DW_EH_PE_sdata2:
> > -    return FdeOff + read16<E>(F.PCRel);
> > -  case DW_EH_PE_udata4:
> > -  case DW_EH_PE_sdata4:
> > -    return FdeOff + read32<E>(F.PCRel);
> > -  case DW_EH_PE_udata8:
> > -  case DW_EH_PE_sdata8:
> > -    return FdeOff + read64<E>(F.PCRel);
> > +  auto Read = [=](uint8_t Size) -> uint64_t {
> > +    const endianness E = ELFT::TargetEndianness;
> > ----------------
> > ruiu wrote:
> >> Do you have to use lambda? It seems you can write without it in a more
> regular way.
> >>
> >>   const endianness E = ELFT::TargetEndianness;
> >>
> >>   int Size = F.Enc & 0x7;
> >>   if (Size == DW_EH_PE_absptr)
> >>     Size = sizeof(uintX_t) == 8 ? DW_EH_PE_udata8 : DW_EH_PE_udata4;
> >>
> >>   uintX_t V;
> >>   switch (Size) {
> >>   case DW_EH_PE_udata2:
> >>     V = read16<E>(F.PCRel);
> >>   case DW_EH_PE_udata4:
> >>     V = read32<E>(F.PCRel);
> >>   case DW_EH_PE_udata8:
> >>     V = read64<E>(F.PCRel);
> >>   default:
> >>     fatal("unknown FDE size encoding");
> >>   }
> >>
> >>   switch (F.Enc & 0x70) {
> >>   case DW_EH_PE_absptr:
> >>     return V;
> >>   case DW_EH_PE_pcrel:
> >>     return V + EhVA + F.Off + 8;
> >>   default:
> >>     fatal("unknown FDE size relative encoding");
> >>   }
> >>
> >> Do you have to use lambda? It seems you can write without it in a more
> regular way.
> >
> > The lambda is not mandatory. But without it the code is a bit longer
> because we have to add `break` to each case of the `switch (Size)`.
> >
> >
> > Repository:
> >   rL LLVM
> >
> > http://reviews.llvm.org/D17733
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160301/32c6ef3b/attachment.html>