Rework memory management #619
Conversation
31d57e3 to
9071436
Compare
src/pcre2_jit_compile.c
Outdated
| private_data_length *= sizeof(sljit_s32); | ||
| /* Align to 32 bit. */ | ||
| total_length = ((re->top_bracket + 1) + (sljit_uw)(sizeof(sljit_s32) - 1)) & ~(sljit_uw)(sizeof(sljit_s32) - 1); | ||
| if (~(sljit_uw)0 - private_data_length <= total_length) |
There was a problem hiding this comment.
nit: I think this could be < rather than <=
There was a problem hiding this comment.
Since max is odd number, and the other two are even, the == cannot happen. Ok, lets change it.
| SLJIT_FREE(common->private_data_ptrs, allocator_data); | ||
| if (common->has_then) | ||
| SLJIT_FREE(common->then_offsets, allocator_data); | ||
| return PCRE2_ERROR_NOMEMORY; |
There was a problem hiding this comment.
With all these repeated FREE statements, it's common to have a block at the end of the function with the cleanup code, which you can jump to with goto. Saves duplicating the cleanup. The cleanup block can have multiple jump targets, with fallthrough, so it cleans up stuff in reverse order to what was allocated. Or it can just check each pointer for NULL and clean up all the ones that have been alloced.
There was a problem hiding this comment.
JIT does not use goto at the moment. But it is reasonable here. I expect compilers to do CSE. But you cannot trust in all compilers. Not sure what is the best. Shall we introduce goto?
There was a problem hiding this comment.
I don't mind; if you don't want to use goto, then duplicating the FREE calls is OK. It's your code - it's only a maintenance concern.
| @@ -13202,22 +13210,28 @@ SLJIT_ASSERT(common->early_fail_start_ptr <= common->early_fail_end_ptr); | |||
| if (private_data_size > 65536) | |||
There was a problem hiding this comment.
I noticed on one of your branches, you have modified the if (private_data_size > 65536) check.
On master, it returns PCRE2_ERROR_NOMEMORY but at one time you changed it to PCRE2_ERROR_JIT_UNSUPPORTED.
It seems odd to report "not enough memory" when you reach that private_data_size limit, since it's not an issue with the machine's RAM, but it appears to be an implementation limitation.
There was a problem hiding this comment.
I don't remember. I have change it.
There was a problem hiding this comment.
It changes the expected file:
-JIT compilation was not successful (no more memory)
+JIT compilation was not successful (feature is not supported by the JIT compiler)
Both seem not entirely correct.
There was a problem hiding this comment.
If the pattern doesn't fit inside the JIT's limits, should it fall back to the interpreter? That would potentially be the main reason to change the return code.
There was a problem hiding this comment.
That happens regardless. Introducing a new error code would be the best.
There was a problem hiding this comment.
If it falls back regardless, then I guess it doesn't matter. Just change it to "PCRE2_ERROR_JIT_UNSUPPORTED" if it's simply a size limitation in the JIT engine.
There was a problem hiding this comment.
Ok. Doing these things could be low hanging fruits for somebody.
e7fa963 to
2849897
Compare
Small code improvement, limit checks.
2849897 to
ec1a04e
Compare
Small code improvement, limit checks.
This patch is another preparation patch, since
private_data_ptrsis available earlier than before.