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