bug#66738: 30.0.50; Gud LLDB regressions

bug#66738: 30.0.50; Gud LLDB regressions

[Gerd Möllmann]

This is with b36e2b1772680b8fce067c6ea2cdf582af982aaa on master.

1. M-x lldb RET leads to errors. *Messages* contains:

error in process filter: gud-marker-filter: Text is read-only
error in process filter: Text is read-only

This seems to come from this code snippet in gud-lldb-marker-filter:

  (let ((bol (pos-bol)))
    (when (> (point) bol)
      ;; Move the current line to the string, so that control sequences
      ;; can delete parts of it.
      (setq string (concat (buffer-substring-no-properties bol (point))
                           string))
      (delete-region bol (point))))

The comment sounds like this code is making wrong assumptions about what
the process buffer contains when it runs. I think this can also be seen
when binding inhibit-read-only around the delete-region. Anyway, I don't
really know what it assumes.

(I'd also find it "cleaner" if the code handling the control sequences
could be moved to a function of its own.)

2. Nitpick: when extracting the file name in the filter function, with

             (group (+ (not (in "\n\r"))))    ; 3: file

this means that file names cannot contain LF and CR, which they actually
can. I guess the only thing a file name definitely cannot contain is
'/'. Don't care about Windows, TBH :-).

bug#66738: 30.0.50; Gud LLDB regressions

[Mattias Engdegård]

> 1. M-x lldb RET leads to errors. *Messages* contains:
> 
> error in process filter: gud-marker-filter: Text is read-only
> error in process filter: Text is read-only

Sorry, I don't observe this myself.

> I think this can also be seen
> when binding inhibit-read-only around the delete-region.

That sounds strange. Did you bind inhibit-read-only to `t`? That should have overridden any text protection.
Unless I can reproduce it, I'm afraid you are left with the task to debug it.

> 2. Nitpick: when extracting the file name in the filter function, with
> 
>              (group (+ (not (in "\n\r"))))    ; 3: file
> 
> this means that file names cannot contain LF and CR, which they actually
> can.

And that is a possibility that I'm going to ignore unless it's actually a security problem (and I don't think it is).
File names containing control characters on purpose are excessively rare in Unix systems and disallowed on Windows (at least 0-31).

> I guess the only thing a file name definitely cannot contain is
> '/'.

/ is trivially allowed in path names; only NUL is disallowed. I'm not sure to what extent we can trust LLDB to normalise line.file.fullpath.

bug#66738: 30.0.50; Gud LLDB regressions

[Gerd Möllmann]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

>> 1. M-x lldb RET leads to errors. *Messages* contains:
>> 
>> error in process filter: gud-marker-filter: Text is read-only
>> error in process filter: Text is read-only
>
> Sorry, I don't observe this myself.

Turns out it's comint-prompt-read-only, but...

>
>> I think this can also be seen
>> when binding inhibit-read-only around the delete-region.
>
> That sounds strange. Did you bind inhibit-read-only to `t`? That should have overridden any text protection.
> Unless I can reproduce it, I'm afraid you are left with the task to
> debug it.

...I'll fix this myself, thank you.

Yeah, I did that. But it seems I have misinterpreted what the intention
of the delete-region is.  (I'm kind of refusing to read the partial
terminal enumlator part of the code :-).

>
>> 2. Nitpick: when extracting the file name in the filter function, with
>> 
>>              (group (+ (not (in "\n\r"))))    ; 3: file
>> 
>> this means that file names cannot contain LF and CR, which they actually
>> can.
>
> And that is a possibility that I'm going to ignore unless it's
> actually a security problem (and I don't think it is).  File names
> containing control characters on purpose are excessively rare in Unix
> systems and disallowed on Windows (at least 0-31).

Then let me ask differently: why did you change this in the first place?

bug#66738: 30.0.50; Gud LLDB regressions

[Mattias Engdegård]

25 okt. 2023 kl. 11.06 skrev Gerd Möllmann <gerd.moellmann@gmail.com>:

> ...I'll fix this myself, thank you.
> 
> Yeah, I did that. But it seems I have misinterpreted what the intention
> of the delete-region is.

The gist of it is that lldb uses CHA and ED as follows:

  "previous line\nsome text" (CHA N) (ED) "something else"

where (CHA N) repositions the cursor to column N, which is usually somewhere inside "some text", and (ED) clears the rest of the text on the line.

This means that if the current line, "some text", is M characters long, then we can simply delete the last M-N characters and remove the CHA and ED sequences which now have done their job, and continue processing.

The snag is that part of "some text" may have already been inserted into the buffer earlier and is thus not part of the current string being filtered. For that reason, we start by removing that part from the buffer and gluing it onto the front of our string so that the CHA and ED operations can act on it.

That text deleted from the buffer may have been write-protected but that's fine; it will be protected again next time it's inserted.

> Then let me ask differently: why did you change this in the first place?

You mean why I didn't use a series of forward slashes as end-delimiter for the full path name? It seemed no less arbitrary than a newline and no more robust. The current solution is straightforward and handles any Unix or Windows file name users will come across.

I did try out NUL as a delimiter but predictably this didn't work (lldb threw an exception, actually).

bug#66738: 30.0.50; Gud LLDB regressions

[Gerd Möllmann]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> 25 okt. 2023 kl. 11.06 skrev Gerd Möllmann <gerd.moellmann@gmail.com>:
>
>> ...I'll fix this myself, thank you.
>> 
>> Yeah, I did that. But it seems I have misinterpreted what the intention
>> of the delete-region is.
>
> The gist of it is that lldb uses CHA and ED as follows:
>
>   "previous line\nsome text" (CHA N) (ED) "something else"
>
> where (CHA N) repositions the cursor to column N, which is usually somewhere inside "some text", and (ED) clears the rest of the text on the line.
>
> This means that if the current line, "some text", is M characters
> long, then we can simply delete the last M-N characters and remove the
> CHA and ED sequences which now have done their job, and continue
> processing.
>
> The snag is that part of "some text" may have already been inserted
> into the buffer earlier and is thus not part of the current string
> being filtered. For that reason, we start by removing that part from
> the buffer and gluing it onto the front of our string so that the CHA
> and ED operations can act on it.
>
> That text deleted from the buffer may have been write-protected but that's fine; it will be protected again next time it's inserted.
>
>> Then let me ask differently: why did you change this in the first place?
>
> You mean why I didn't use a series of forward slashes as end-delimiter
> for the full path name? It seemed no less arbitrary than a newline and
> no more robust. The current solution is straightforward and handles
> any Unix or Windows file name users will come across.
>
> I did try out NUL as a delimiter but predictably this didn't work (lldb threw an exception, actually).

Ok, thanks.

I see you fixed this in master, so I'm closing this issue.

bug#66738: 30.0.50; Gud LLDB regressions

[Mattias Engdegård]

25 okt. 2023 kl. 16.02 skrev Gerd Möllmann <gerd.moellmann@gmail.com>:

> I see you fixed this in master, so I'm closing this issue.

Yes, sorry about my impatience, and thanks for confirming that the change solved the bug that I introduced.