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

[Marshall Clow via cfe-commits]

mclow.lists added inline comments.


================
Comment at: src/cxa_personality.cpp:363
+           "Unexpected TTypeEncoding");
     (void)ttypeEncoding;
 
----------------
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)


https://reviews.llvm.org/D24085