Skip to content

Commit

Permalink
In Storable.xs, simplify and inline the macro STORE_HASH_SORT.
Browse files Browse the repository at this point in the history
For 5.6.x and earlier, the interpreter context saving/restoring is not needed
prior to calling qsort(), as within the thread there is only one interpreter
in play.

Moreover, the SAVESPTR(orig_perl); in the removed code is not just pointless,
it's technically undefined behaviour. All that the SAVESPTR() macro will do
is ensure that the value of *(&orig_perl) is restored at scope exit. It
won't actually restore the interpreter context - that would need a call
to PERL_SET_CONTEXT with the value of origperl. The undefined behaviour is
because the LEAVE; which causes the scope pop action happens outside the
block, hence orig_perl has gone out of scope, and the address &orig_perl
(saved on the scope stack, written to by LEAVE), now points to invalid
memory. It is, however, quite hard to even get gcc's ASAN to trap this.

This code is likely unreachable anyway. It's only compiled in for 5.6.x and
earlier with USE_ITHREADS. ithreads was not useful until 5.8.0.
  • Loading branch information
nwc10 committed Jun 12, 2013
1 parent f4977f1 commit 5fa0439
Showing 1 changed file with 5 additions and 34 deletions.
39 changes: 5 additions & 34 deletions dist/Storable/Storable.xs
Expand Up @@ -829,37 +829,6 @@ static void bless_retrieved(pTHX_ retrieve_cxt_t *retrieve_cxt, SV *sv, const ch
#define BLESS(s,p) \
(bless_retrieved(aTHX_ retrieve_cxt, s, p))

/*
* sort (used in store_hash) - conditionally use qsort when
* sortsv is not available ( <= 5.6.1 ).
*/

#if (PATCHLEVEL <= 6)

#if defined(USE_ITHREADS)

#define STORE_HASH_SORT \
ENTER; { \
PerlInterpreter *orig_perl = PERL_GET_CONTEXT; \
SAVESPTR(orig_perl); \
PERL_SET_CONTEXT(aTHX); \
qsort((char *)AvARRAY(av), len, sizeof(SV *), sortcmp); \
} LEAVE;

#else /* ! USE_ITHREADS */

#define STORE_HASH_SORT \
qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp);

#endif /* USE_ITHREADS */

#else /* PATCHLEVEL > 6 */

#define STORE_HASH_SORT \
sortsv(AvARRAY(av), len, Perl_sv_cmp);

#endif /* PATCHLEVEL <= 6 */

static void store(pTHX_ store_cxt_t *store_cxt, SV *sv);
static SV *retrieve(pTHX_ retrieve_cxt_t *retrieve_cxt, const char *cname);

Expand Down Expand Up @@ -1469,9 +1438,7 @@ static void store_array(pTHX_ store_cxt_t *store_cxt, AV *av)
static int
sortcmp(const void *a, const void *b)
{
#if defined(USE_ITHREADS)
dTHX;
#endif /* USE_ITHREADS */
return sv_cmp(*(SV * const *) a, *(SV * const *) b);
}

Expand Down Expand Up @@ -1596,7 +1563,11 @@ static void store_hash(pTHX_ store_cxt_t *store_cxt, HV *hv)
av_store(av, AvFILLp(av)+1, key); /* av_push(), really */
}

STORE_HASH_SORT;
#if (PATCHLEVEL <= 6)
qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp);
#else
sortsv(AvARRAY(av), len, Perl_sv_cmp);
#endif

for (i = 0; i < len; i++) {
unsigned char flags = 0;
Expand Down

0 comments on commit 5fa0439

Please sign in to comment.