Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows fixes #4

Merged
merged 4 commits into from
Apr 17, 2023
Merged

Windows fixes #4

merged 4 commits into from
Apr 17, 2023

Conversation

michaliskambi
Copy link

The LSP server was always crashing on Windows with Access Violation.

The core culprit was: one cannot use logic like

URI := ParseURI(UriStr);
FileName := URI.Path + URI.Document;

on Windows. It will result in an additional forward slash being added at the beginning of the filename, and the resulting path is not a valid filename on Windows, that can be opened. For example the filename was /d:/cygwin64/home/michalis/installed/fpclazarus/3.2.2-lazarus2.2/lazarus/components/codetools/unitdictionary.pas.

utextdocument unit used this faulty logic a few times. In 3x cases there was a check whether the file was opened successfully with a clear exception, but the TextDocument_DidOpen and TextDocument_DidChange did

Code := CodeToolBoss.LoadFile(..., false, false);
Code.Source := Content;

which crashes with Access Violation when filename was invalid, because CodeToolBoss.LoadFile then returns nil.

I fixed the core issue, and made some related things more reliable and easier to debug (this should help also on non-Windows):

  1. To convert URI to a filename, URIParser standard unit contains a ready URIToFilename routine. This deals with all cross-platform stuff for us. I changed the code to use it everywhere.

    ( Castle Game Engine is using URIToFilename too, for all our URI->filename logic, through https://github.com/castle-engine/castle-engine/blob/master/src/files/castleuriutils.pas#L910 . It is reliable. )

  2. Just in case, I added check for Code = nil after Code := CodeToolBoss.LoadFile. It should not fail anymore, but if it will -- it will show a nice exception like FATAL EXCEPTION: Unable to load file .... instead of cryptic Access Violation.

  3. In castle-engine@78c7163 I extended the exception message with a backtrace. This uses DumpExceptionBackTrace from FPC, wrapped in easy function DumpExceptionBackTraceToString (from Castle Game Engine https://github.com/castle-engine/castle-engine/blob/master/src/base/castleclassutils.pas#L2257 , here simplified a little).

    In the end you will get better exception messages with backtrace in the log, like this:

    FATAL EXCEPTION: Unable to load file /d:/cygwin64/home/michalis/installed/fpclazarus/3.2.2-lazarus2.2/lazarus/components/codetools/unitdictionary.pas
    
      $000000010004565D  TEXTDOCUMENT_DIDOPEN,  line 100 of utextdocument.pas
      $0000000100001B5B  DISPATCH,  line 53 of pasls.lpr
      $0000000100001D76  MAIN,  line 90 of pasls.lpr
      $0000000100002612  main,  line 239 of pasls.lpr
      $0000000100002716  main,  line 255 of pasls.lpr
      $0000000100014830
      $00000001000019B0
      $00007FFA232174B4
      $00007FFA23A226A1
    
  4. In castle-engine@4767a32 I also did a few fixes to have the DebugLog output use "native" line endings, i.e. #13#10 on Windows, #10 on Unix. Previous code was using #10 in most places, which is non-standard for Windows, and also was getting mixed with #13#10 produced by DumpExceptionBackTrace on Windows.

    So to have consistent line endings, standard on Windows, I use LineEnding everywhere.

  5. Finally in castle-engine@f6410d1 I changed the code to use FileNameToURI, instead of doing it by 'file://' + CurPos.Code.Filename .

    On Windows, the previous "manual" method of concatenation generates invalid URL, as it misses an additional slash. See https://en.wikipedia.org/wiki/File_URI_scheme#Windows_2 how the file URL on Windows should look like.

    The practical effect of this problem was that jumping to identifiers, e.g. by Ctrl + click on a method name in VS Code, wasn't working (VS Code reported error as on the screenshot). After the fix, it works perfectly.

    vsc

Note: Originally I did these changes on https://github.com/castle-engine/pascal-language-server branch that adds also CGE-specific modifications and support for log file defined in c:/Users/<username>/AppData/Local/pasls/castle-pasls.ini file. This made the debugging even easier, I saw the FATAL EXCEPTION... error in a regular text file. But I don't submit it with this PR, I'm myself not sure about the elegance of this INI file support (it seems cleaner to get all options through LSP initialization options, not some additional INI file).

Tested with Emacs on Windows and VS Code on Windows. After the fixes, they work as nicely as on Linux :)

Allows to easily debug many cases by just looking at the log.
Using "URI.Path + URI.Document" to convert TURI to a filename is not good on Windows, as it results in invalid filenames like "/d:/cygwin64/home/michalis/installed/fpclazarus/3.2.2-lazarus2.2/lazarus/components/codetools/unitdictionary.pas".

The URIToFilename from URIParser unit instead provides a cross-platform correct solution.

Also, we improve error reporting in case CodeToolBoss.LoadFile fails: clear error message instead of Access Violation at Code.Source usage.
@Isopod Isopod merged commit 4a4ea73 into Isopod:master Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants