logan added inline comments.
================
Comment at: src/cxa_personality.cpp:363
+ "Unexpected TTypeEncoding");
(void)ttypeEncoding;
----------------
mclow.lists wrote:
> logan wrote:
> > mclow.lists wrote:
> > > It's not clear to me how this accomplishes what you want.
> > > You're looking for `00/10/90`, right? Why not just check for that?
> > >
> > > Why are you anding with 0x0f ?
> > > Before, this would pass only a single value - `DW_EH_PE_absptr` (aka 0)
> > > With this change, it passes 32 values: 00, 03, 10, 13, 20, 23, and so on.
> > >
> > > Was that your intent?
> > >
> > `ttypeEncoding` is encoded with following rules:
> >
> > 1. Lower 4 bits stand for the representation of the data, such as `absptr`,
> > `uleb128`, `udata1`, `udata2`, `udata4`, `udata8`, etc. These bits control
> > the way we extract the bytes from the exception table.
> >
> > 2. Upper 4 bits stand for the post processing action, such as `pcrel`,
> > `indirect`, etc. For example, if `pcrel` is specified, then we should add
> > the value, which was read in step 1, with the address of the value.
> >
> > My intention is to weaken the assertion (only assert the essential
> > assumption) so that we don't have to alter the assertion if there are new
> > configurations that I am not aware of or new compiler which is generating
> > different ttypeEncoding.
> >
> > Since the upcoming lines (L365) only uses `sizeof(uintptr_t)` to decode the
> > TType pointer, it is not necessary to impose restriction on the upper 4
> > bits. That's the reason why I wrote `ttypeEncoding & 0xf`. For the same
> > reason, both `absptr` and `udata` have the same meaning (4 bytes in the
> > exception table) in this context, thus I am adding extra `(ttypeEncoding &
> > 0x0f) == DW_EH_PE_udata4`.
> Ok; thanks for the explanation. However, I'm still concerned.
> The assert is there to catch bad assumptions. (i.e, this can never happen).
>
> the old code would assert on 255 of 256 possible values.
> It turns out that this is too strict - there are at least three values that
> we need to pass.
>
> But your assertion passes 32 possible values (out of 256). So if something
> goes wrong, and a random value for `ttypeEncoding` gets passed in here,
> there's a 1 in 8 chance that the assertion will not fire. *Thats* my
> concern. It should never be an issue, because we don't have bugs, and never
> pass random values around, right? ;-)
>
> As for "dealing with new configurations" or "new compilers", I would say
> those are very infrequent events; and I wouldn't worry about them until they
> happen. (Demonstrations that I am misinformed here are welcome)
Thanks. I'll update the patch soon.
https://reviews.llvm.org/D24085
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
