SQLite Forum

shell's .load is pathSep picky (bug, now fixed)
Login

shell's .load is pathSep picky (bug, now fixed)

(1.1) By Larry Brasfield (LarryBrasfield) on 2020-04-26 23:44:24 edited from 1.0 [source]

I see that the bug I described in this thread, and for which a simple fix is shown in that same post, remains in the source for loadext.c .

If it is not a bug, then the documentation for the API and SQL functions that reach sqlite3LoadExtension() needs a note added regarding the required path separator, which is always '/' for the last one now when the entry point default is taken and the given DLL location has more than a basename.

If it has been recorded as a bug, I missed noticing that. (I need to learn ticket searching skills.)

Amended 2020-04-26: Now fixed on trunk as of 22:04 check-in.

(2.1) By Larry Brasfield (LarryBrasfield) on 2020-04-26 15:23:32 edited from 2.0 in reply to 1.0 [link] [source]

Check-in bc3bf7c6 appears to be intended as a fix for the subject bug, but it does not work as intended.

The hunt for beginning of the DLL basename when SQLITE_OS_WIN is true now reads:

    for(iFile=ncFile-1; iFile>=0 && ((c=zFile[iFile]!='/')||c=='\\'); iFile--){}

This has two problems: (1) The search should stop, rather than continue, when the usual Windows path separator '\' is found; and (2) the value of c becomes the boolean result of the comparison to '/', which is always false when the comparison to '\\' is reached thus being equivalent to 0=='\\' for that test.

The intent of that code would be better expressed with this replacement:

    for(iFile=ncFile-1; iFile>=0 && (c=zFile[iFile])!='/'&&c!='\\'; iFile--){}

I have built and tested both versions with these shell commands, (where the shell executable is not located in /Bin), with results as noted:

  .load /Bin/natsort
  .load \\Bin\\natsort sqlite3_natsort_init
These two succeed for all versions.

  .load \\Bin\\natsort
This fails with "Error: The specified procedure could not be found." for check-in bc3bf7c6 (and earlier versions) and succeeds with the proposed replacement.

(3.1) By Larry Brasfield (LarryBrasfield) on 2020-04-26 15:24:38 edited from 3.0 in reply to 2.0 [link] [source]

I almost added that a good optimizer would eliminate the 0=='\\' test, but decided that is irrelevant to this thread. However, out of curiosity about the MSVC v19 code generator given optimization flags, I looked at assembler output for both the bc3bf7c6 check-in and the proposed replacement. Here are the results, lightly annotated in comments:

; Original code:
; Line 123565
	lea	r10d, DWORD PTR [rbx-1]
	mov	QWORD PTR [rax], rcx
	mov	ecx, ebx
	sub	rcx, 1
	js	SHORT $LN205@sqlite3Loa
$LL7@sqlite3Loa:
	cmp	BYTE PTR [rcx+rsi], 47			; '/'
	je	SHORT $LN205@sqlite3Loa
	dec	r10d
	sub	rcx, 1
	jns	SHORT $LL7@sqlite3Loa
$LN205@sqlite3Loa:

; Revised code:
; Line 123567
	lea	r10d, DWORD PTR [rbx-1]
	mov	QWORD PTR [rax], rcx
	mov	ecx, ebx
	sub	rcx, 1
	js	SHORT $LN203@sqlite3Loa
	npad	7
$LL7@sqlite3Loa:
	movzx	eax, BYTE PTR [rcx+rsi]
	cmp	al, 47					; '/'
	je	SHORT $LN203@sqlite3Loa
	cmp	al, 92					; '\\'
	je	SHORT $LN203@sqlite3Loa
	dec	r10d
	sub	rcx, 1
	jns	SHORT $LL7@sqlite3Loa
$LN203@sqlite3Loa:

As can be seen in the 'cmp al, ?' instructions, the 2nd test is optimized out when it could not succeed. The optimizer recognized this, not via constant expression analysis but by analyzing the possible values of c when the 2nd test was reached.

As can also be seen, the possibly performant conversion of code that might be written:

  zFile[iFile]!='/'&&zFile[iFile]!='\\'

into something that avoids an indexing operation:

  (c=zFile[iFile])!='/'&&c!='\\'

is not needed with this particular optimizing compiler. I dare say that recognizing common subexpressions such as 'zFile[iFile]' is an easier and more common optimizer feature than the one demonstrated in the above assembler output.

Of course, this code runs after a DLL has been loaded, after some file reads and many thousands of instructions run to (dynamically) link it, so the possible elimination of a load instruction is like a sneeze in a hurricane.

(4) By Larry Brasfield (LarryBrasfield) on 2020-04-26 21:22:23 in reply to 2.1 [link] [source]

I am sorry to have to report that check-in 57b16d8c is still not right.

Put briefly, the newest change's code fragment,

(c=zFile[iFile]!='/')

, assigns the comparison result, a truth value (1 or 0), to c rather than the char value that was probably intended. If that fragment was instead written,

(c=zFile[iFile])!='/'

, then c would be assigned the char value zFile[iFile] and the code then comparing c to '\\' would work.

If that is accepted as a fact, the rest of this post can be ignored because it only goes to prove what I claim above and show, in detail, why it it is true.

// I modified the subject code taken from loadext.c (in the amalgamation sqlite3.c) to read:

    memcpy(zAltEntry, "sqlite3_", 8);
#if SQLITE_OS_WIN
# if defined(FAVOR_DRH_LOAD_EXT_ENTRY_FIX1)
    for(iFile=ncFile-1; iFile>=0 && ((c=zFile[iFile]!='/')||c=='\\'); iFile--){}
# elif defined(FAVOR_DRH_LOAD_EXT_ENTRY_FIX2)
    for(iFile=ncFile-1; iFile>=0 && ((c=zFile[iFile]!='/')&&c!='\\'); iFile--){}  /* This is line 123567 */
# else
    for(iFile=ncFile-1; iFile>=0 && (c=zFile[iFile])!='/'&&c!='\\'; iFile--){} /* Note movement of 2nd ')' */
# endif
#else
    for(iFile=ncFile-1; iFile>=0 && zFile[iFile]!='/'; iFile--){}
#endif
    iFile++;

I added this in Makefile.msc where it's timely:

#  Temp defs to test bug fix options
!IF defined(BUGFIX_OPT)
TCC = $(TCC) -DFAVOR_DRH_LOAD_EXT_ENTRY_FIX$(BUGFIX_OPT)
!ENDIF

Here are excerpts from a subsequent OS shell session, minus much extraneous matter:

> build BUGFIX_OPT=0 clean
...
> build BUGFIX_OPT=2
... cl ... -DFAVOR_DRH_LOAD_EXT_ENTRY_FIX2 ...
> sqlite3m -bail -cmd ".load \\Bin\\natsort"
Error: The specified module could not be found.

> build BUGFIX_OPT=0 clean
...
> build BUGFIX_OPT=1
... cl ... -DFAVOR_DRH_LOAD_EXT_ENTRY_FIX1
> sqlite3m -bail -cmd ".load \\Bin\\natsort"
Error: The specified procedure could not be found.

> build BUGFIX_OPT=0 clean
...
> build BUGFIX_OPT=0
... cl ...  -DFAVOR_DRH_LOAD_EXT_ENTRY_FIX0
> sqlite3m -bail -cmd ".load \\Bin\\natsort"
SQLite version 3.32.0 2020-04-23 20:45:46 (with modified shell)
Enter ".help [<command>]" for usage hints.
sqlite> .q

> alias build
nmake -f Makefile.msc SESSION=1 SQLITE3EXE=sqlite3m.exe DYNAMIC_SHELL=1 OPTIMIZATIONS=4 DEBUG=0 SYMBOLS=0 TEMP_STORE=2 OPT_FEATURE_FLAGS="-DSQLITE_DQS=0 -DSQLITE_ENABLE_FTS4=1 -DSQLITE_ENABLE_FTS5=1 -DSQLITE_ENABLE_JSON1=1 -DSQLITE_ENABLE_RTREE=1 -DSQLITE_ENABLE_COLUMN_METADATA=1 -DSQLITE_DEFAULT_FOREIGN_KEYS=1 -DSQLITE_ENABLE_GEOPOLY -DSQLITE_ENABLE_SESSION -DSQLITE_ENABLE_PREUPDATE_HOOK=1 -DSQLITE_INTROSPECTION_PRAGMAS -DSQLITE_ENABLE_EXPLAIN_COMMENTS -DSQLITE_ENABLE_STMTVTAB -DSQLITE_ENABLE_DBPAGE_VTAB -DSQLITE_ENABLE_DBSTAT_VTAB -DSQLITE_ENABLE_UNKNOWN_SQL_FUNCTION -DSQLITE_USE_URI -DSQLITE_OMIT_DEPRECATED -DSQLITE_USE_ALLOCA -DSQLITE_DEFAULT_SYNCHRONOUS=3 -DSQLITE_LIKE_DOESNT_MATCH_BLOBS -DSQLITE_DEFAULT_WORKER_THREADS=3" SHELL_EXTRA_OPTS="-Ox -I..\..\zlib-1.2.11 -DBLOB_IO=1 -DTSV_INIT=1 -DUSE_SYSTEM_SQLITE=1 -DSQLITE_ENABLE_SESSION=1 -DSQLITE_HAVE_ZLIB=1 -DNO_SELFTEST -DSQLITE_UNTESTABLE" LDOPTS="C:\Work\Projects\zlib-1.2.11\zlib.lib /NODEFAULTLIB:MSVCRT /INCREMENTAL:NO"

Here are excerpts from the assembly output with -DFAVOR_DRH_LOAD_EXT_ENTRY_FIX2 compilation:

_TEXT	SEGMENT
...
sqlite3LoadExtension PROC
; File C:\Work\Projects\Sqlite\v3r32pr6\sqlite3.c
...
; Line  123567
	lea	r10d, DWORD PTR [rbx-1]
	mov	QWORD PTR [rax], rcx
	mov	ecx, ebx
	sub	rcx, 1
	js	SHORT $LN203@sqlite3Loa
$LL7@sqlite3Loa:
	cmp	BYTE PTR [rcx+rsi], 47			; 0000002fH
	je	SHORT $LN203@sqlite3Loa
	dec	r10d
	sub	rcx, 1
	jns	SHORT $LL7@sqlite3Loa
$LN203@sqlite3Loa:

; Note that above comparison to 47 (aka '/') is to an indirectly accessed location. Also note that there is no comparison to 92 (aka '\') anywhere. The code that might do so has been optimized away because this

  && ((c=zFile[iFile]!='/')&&c!='\\')

is logically equivalent to this appearing the top of the loop body:

  if( !( c=zFile[iFile]!='/' ) ){  /* 1st test */
    if( !( c!='\\' ) )              /* 2nd test */
      break;
  }

The compiler, enforcing the higher binding precedence of != relative to =, "stores" 1st comparison truth value into what would be c. (The quotes are because, in fact, the "c" value remains enregistered, not stored, as the above .asm excerpts show.) Because the 2nd test can only be reached when c would have been given the truth value "False", or 0 per C convention, and because 0!='\' is always true and the 2nd test condition can never be met, there is nothing achieved by executed such code with a constant outcome. And so it is optimized away.

Here, with the troublesome clause selected (at line 123569) as

  && (c=zFile[iFile])!='/'&&c!='\\' 

, are excerpts from the assembly output:

; Line 123569
	lea	r10d, DWORD PTR [rbx-1]
	mov	QWORD PTR [rax], rcx
	mov	ecx, ebx
	sub	rcx, 1
	js	SHORT $LN203@sqlite3Loa
	npad	7
$LL7@sqlite3Loa:
	movzx	eax, BYTE PTR [rcx+rsi]
	cmp	al, 47					; 0000002fH
	je	SHORT $LN203@sqlite3Loa
	cmp	al, 92					; 0000005cH
	je	SHORT $LN203@sqlite3Loa
	dec	r10d
	sub	rcx, 1
	jns	SHORT $LL7@sqlite3Loa
$LN203@sqlite3Loa:
; Note that above comparisons to 47 and 92 (aka '/' and '\\') are to a loaded register.

This last is the code that works as demonstrated by the shell session excerpts. It ends the loop if either of two successive comparisons are met.

I apologize if this is excessively belaboring the obvious. My first fix was not the one I first suggested, so I am sympathetic regarding the confusion.

(5) By Larry Brasfield (LarryBrasfield) on 2020-04-26 23:39:00 in reply to 4 [link] [source]

Check-in b73d9a7d6f7fec0f, using new DirSep(c) macro, works great. Thanks.