[PATCH] D24085: arm: Fix ttype encoding assertion failure.

[Logan Chien via cfe-commits]

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