FpDebug: Bugs and API gaps, with proposed fixes

[MattBradford]

Thanks for taking G8 the rest of the way, and for doing the line-info work yourself. I tested fefc5135 before you merged: all seven of my by-name cases came back byte-identical to my own branch, and the returned symbol now carries FileName, Line, LineStartAddress and LineEndAddress, all agreeing with what FindProcSymbol(Address) gives for the same procedure. On my previous run that line read "file= line=0", so the lazy resolution does exactly what you designed it to.

Three things turned up afterwards while I was preparing the TestFpDebugApi fixtures. None is a bug report; two are questions about what the intended behaviour is, because a test has to assert one thing or the other.

1. The returned symbol's Name echoes the spelling the caller asked with

Same image, same session, the only difference being the library. A temporary probe calling FindNamedProcSymbol directly and printing what the returned symbol carries, against a source-level "procedure FooBar":

Code: [Select]

request    before your refactor        after
FooBar     name=FooBar                 name=FooBar
FOOBAR     name=FooBar                 name=FOOBAR
fOobAR     name=FooBar                 name=fOobAR
Class, kind, address, file, line, line-start and line-end are identical in all six. Only Name differs. The mechanism looks like CreateSubClass(AName, InfoEntry) carrying the requested name, where the by-address path takes it from the DIE.

Which is intended? I can see the argument for either: echoing what was asked is honest about what the caller passed, and the DWARF spelling is what a UI would want to display. It matters to me only because the round-trip assertion we agreed on goes through FindProcSymbol(Address) and is unaffected, but an assertion of the form "the symbol found by name reports the source spelling" would pass before and fail after, and I would rather write the one you want than pick.

2. A procedure nested inside a method resolves by name

Measured on the WatchesScopePrg testapp: "probe MethodMainChildNested" returns skProcedure with file and line, identically before and after your refactor. My MR description said results were "limited to global procedures", which on this evidence is not accurate.

Is that supported behaviour I should test for, or does it just fall out of the scan and might change? I have not chased how far it goes - nested-inside-nested, or whether the address is usable for a breakpoint - because that depends on the answer.

3. An interface constant left fpdbginfo.pas

FpProcSearchNameSpaces is now a local const inside ProcSearchIncludes. Nothing I have references it and the out-of-tree consumer does not either, so this costs nobody anything that I know of - flagging it only because an interface constant disappearing is the kind of thing a consumer finds at compile time and wonders about. No action wanted.

On the testsuite fixtures

You asked whether they could go into an existing testapp rather than adding exes. They can. I built a copy of WatchesScopePrg with three procedures inserted before TestFin - one named after the program itself, one for the case-matching test, and an FPC_BREAK_ERROR shadow called behind a condition that is never true - and it compiles clean with no warnings and probes exactly as my three standalone programs did. The file marks its breakpoints with TEST_BREAKPOINT comments rather than line numbers and its header says insertion is allowed, so nothing existing moves.

The one case that cannot go in there is the scoped-enum collision: {$ScopedEnums on} is per-unit and would change the meaning of every existing enum reference in that file. That one needs a small new unit through TEST_USES - a source file, but no new exe.

If that shape suits you I will put the tests together on it, once I know which way you want question 1 to go.

[Martin_fr]

The returned symbol's Name echoes the spelling the caller asked with

Code: Pascal  [Select][+][-]

1. function TFpDwarfInfoSymbolScope.FindExportedSymbolInUnits(const AName: String;
2.  
3.     ADbgSymbol := TFpSymbolDwarf.CreateSubClass(AName, FoundInfoEntry);
4.  
5. function TFpDwarfInfoSymbolScope.FindExportedSymbolInUnits(const AName: String;
6.  
7.   Result := FindExportedSymbolInUnits(AName, ANameInfo, SkipCompUnit, FoundInfoEntry,
8.       OnlyUnitNameLower, AFindFlags);
9.  

Try empty string instead of "AName" => then it should be looked up when needed.

But, we may need a new flag for that. Need to be checked. If (not sure, but if) other callers call with a name already found from debug info (i.e. know to be correct), then that should not be discarded at the cost of a new lookup.

Also the flag may be passed in by normal callers, if only the address is needed. Albeit in that case it wouldn't matter, because the name is only looked up on demand (afaik)

A procedure nested inside a method resolves by name

That is what FPC (as producer of the info) gives us.

It writes all procedures top level (except anonymous). There is also no info in which procedure it is nested.
This is also true for static class methods (afaik / must check / at least for class var)

There is no saying, if or when or why FPC may change that.

At current it is possible (would need to go into the FreePascal extension unit) to check for the parameter "$parent_frame" (or similar), if that param is present its a nested proc.

And if paused in such a procedure, then by following the stack (in combination with the value of the parent_fp) the outer procedure(s) can be found. But a valid callstack is needed for that.


The address should be usable for a breakpoint. In that it does not differ from any other proc. It just means you are more likely to have name-clashes.

I would have to check (objdump/dwarfviewer) if there is a flag to determine if a proc is in the interface section or implementation only

An interface constant left fpdbginfo.pas

"left" may be the wrong term.

It only existed there in the MR branch. It was added by you:
Revision: 97463cbce57572576329d4143a1a9d63a8032665
Author: Mathew Bradford <brgit@pinnaclesimulation.com>
Date: 15/08/2026 21:07:21

So, it never existed before. If it becomes a requirement to provide such a constant for the outside world, then that can be moved.

On the testsuite fixtures

Sounds all fine.

But are you sure ScopedEnum is global?

The below compiles, with 3.2.0 and 3.2.3 and 3.3.1

Code: Pascal  [Select][+][-]

1. program Project1;
2. {$Mode objfpc}
3. type
4.   {$SCOPEDENUMS OFF}
5.   t1 =(a,b);
6.   {$PUSH}
7.   {$SCOPEDENUMS On}
8.   t2 =(c,d);
9.   {$POP}
10.   t3 =(e,f);
11.   {$SCOPEDENUMS OFF}
12.   t4 =(g,h);
13.  
14. var
15.   a1: t1;
16.   a2: t2;
17.   a3: t3;
18.   a4: t4;
19. begin
20.   a1 := a;
21.   //a2 := c; //this fails, this is the only one that fails
22.   a2 := t2.c;
23.   a3 := e;
24.   a4 := g;
25. end.
26.  

[MattBradford]

CheckForConsoleOutput: ATimeOutMs is inert on the registered-reader path -- doc note, or signature change?

This is a question rather than a defect report. I think the current behaviour is the right one, and I am not asking for it to change.

TDbgWinProcess.CheckForConsoleOutput has two branches. Read on main at bb32278d84:

Code: [Select]

if (_CancelSynchronousIo <> nil) and (CheckingForConsoleOutputThread <> nil) then begin
  if FGetConsoleBufferNeedSleep then sleep(10);
  try
    FGetConsoleBufferCnt := FProcProcess.Output.Read(FGetConsoleBuffer, 1);   // blocking
    ...
  exit;
end;

Deadline := SysUtils.GetTickCount64 + QWord(ATimeOutMs);
repeat
  ...
until StopCheckingForConsoleOutputRequested or (SysUtils.GetTickCount64 >= Deadline);

The first branch never reads ATimeOutMs. It parks in a blocking one-byte read, abortable only through _CancelSynchronousIo, which StopCheckingForConsoleOutput does call. Only the second branch honours the timeout, and it is unreachable for any caller that has registered a reader thread through SetCheckingForConsoleOutputThread -- which is what the IDE does, and what we do.

To be clear which way I think this should go: the blocking read looks better for the purpose. It wakes when the target writes instead of at a poll boundary, and the cancel path is clean. I am not proposing that polling comes back.

What I am flagging is narrower: a parameter that is still in the signature is now inert for those callers, and nothing says so. That cost us something real. Our pump passed 250 for weeks in the belief that it bounded the wait, and our own notes said "the pump wakes every 250 ms" when in fact it parks until the target speaks. Our mistake to make, but the signature invited it.

So, whichever you prefer:

• a comment on the method saying the parameter is ignored when a reader thread is registered;
• a signature or naming change that makes it structural rather than documented;
• or nothing, on the grounds that the reader-thread contract already implies it.

Happy to write any of them, or to leave it alone. I have no stake beyond not wanting the next reader to take the parameter the way we did.

One smaller thing while in that method, and this one is mine rather than yours: the comment "Launched without pipe capture -> report no console" now sits above the timeout loop, but that case is handled earlier by the FProcProcess / Output nil test, and what actually selects the loop today is the absence of a registered reader thread. I wrote that comment before the first branch existed, so it is my stale comment, not a report about your change.

[Martin_fr]

About CheckForConsoleOutput...

First of all, when I have a lot of time, I need to check if that extra thread should be created by the external consumer, or should be part of FpDebug's TDbgWinProcess.

But in either case

a comment on the method saying the parameter is ignored when a reader thread is registered;

Is not true. Depends on the Windows version too. Or more to the point the availability of _CancelSynchronousIo.

And, the factors on which it depends may vary in future. And other OS may also get/loose those.

So a comment should just generally state (possible in fewer words)

Code: Text  [Select][+][-]

1. ATimeOutMs:
2. The function may freely chose to honor or ignore this value.
3. (e.g. the function may decide depending on if it can't
4. (or wont "choses not to") perform blocking reads)
5. - If the function does not honour the timeout,
6.   it will still have to be able to react to requests
7.   of cancellation or exposed functionality relates
8.   to console reads.
9. - If the timeout is honored, it may or may not cause
10.   the function to return to the caller periodically
11.   (at the requested or any other interval).
12.   In case that it does return to the caller, it would
13.   expect to be invoked again in order to continue.
14. The function could also use the timeout in any other manner
15. to ensure it complies with related requests, such as checking
16. at the given interval and continue internally.
17.  

I know this is vague. But I'd rather force people to ask, than to promise something that might not be hold up.

If a finer contract would be needed, then an API would need to be added for one or more of the following
- force honoring timeout / disable other ways
- question if timeouts would be dishonoured (and potentially in which way / e.g. if a cancellation would be immediate, or may take up to timeout ms)

Unless its urgently needed, I don't feel like adding that now.

a signature or naming change that makes it structural rather than documented;
or nothing, on the grounds that the reader-thread contract already implies it.

I don't think the signature should change right now. Especially given that the ownership of that thread may currently be wrong. (I haven't double checked that now, just when I looked now with some days distance to the original commit)

So a comment I think it shall be.

But as in my example, generic. The details of when and how are not part of it. They may change.



That said, yes I am aware the API of Fpdebug needs in some way to be finalized, and (ah well) documented. (not sure if that should be by comment, I may prefer FpDoc)

But documenting means to ensure the interface is correct.

FpDebug has been started in 2006. It has gone through many hands. And I have to admit, that while I worked on it, lots has been considered later than it should have. Some because of details being simple discovered that late. Others because to some extend it was more important to get it to work at all.

the comment "Launched without pipe capture -> report no console"

Before I dig may way through the files history to see what happens => the end is just: remove that comment?

If so, then I will just do that, and commit it.

I can add above comment at the same time.

[Martin_fr]

I have to spend some more time on that.

I.e. the comment should probably define a relation to

- either return after the timeout, or earlier to allow the caller to stop calling it
- check cancel request by at least that frequency
- be able to react immediately to cancel requests
- provide other means to cancel that will take no longer than the timeout (plus normal processing time)

"cancel" serves as example for all interactions.

[MattBradford]

Yes - please remove that comment, and my proposed wording was wrong.

Taking your direct question first, since you offered to commit it: yes, the end is just "remove that comment", and nothing needs to replace it.

It is the two-line comment sitting immediately above Deadline := SysUtils.GetTickCount64 + QWord(ATimeOutMs); in TDbgWinProcess.CheckForConsoleOutput. Three reasons it should simply go:

• The case it names - launched without pipe capture - is handled about twenty lines earlier, by the (FProcProcess = nil) or (FProcProcess.Output = nil) test that exits -1.
• It is wrong about the branch it now sits on: that loop returns Avail or 0, and never a negative except through the exception path.
• It is mine, from before the first branch existed. Not a report about your change.

Please do remove it and commit. Read, not measured - I traced it on bb32278d84 and did not rebuild.

On my proposed wording

You are right and I will not defend it. "The parameter is ignored when a reader thread is registered" names the wrong condition: the test in the source is (_CancelSynchronousIo <> nil) and (CheckingForConsoleOutputThread <> nil), and I quoted back only the half that happened to be constant on my machine. On my build _CancelSynchronousIo always resolves, so the reader thread looked like the whole story. Generalising from a single configuration is a mistake I have made on this thread before, and this is the same one.

On the contract comment

Agreed on the approach - a comment that promises less than the implementation currently does is the right trade, and I would rather be forced to ask than to rely on something that quietly stops being true.

You asked for fewer words. One attempt, entirely take-it-or-leave-it:

Code: [Select]

ATimeOutMs:
  The implementation may honour or ignore this value - it may, for
  example, block until data arrives rather than poll. Either way it
  must stay responsive to cancellation and to related requests within
  that interval. If it does honour the timeout it may, but need not,
  return to the caller periodically, expecting to be called again.

Your four points in the follow-up are the load-bearing part of it, and I have tried to keep the "either return, or check at least that often, or provide another means bounded by it" shape rather than pin it to one of them. If it has lost something, use yours - the vagueness is deliberate on your side and I would rather not sand it off by accident.

On the reader thread's ownership

Noted, and nothing is being asked for on our side. We are not blocked either way: our pump passed 250 believing it bounded the wait, and now that we know it does not, it costs us nothing - the blocking read is better for us than the poll was. I raised it so the next reader would not repeat the assumption, and a comment does that.

[Martin_fr]

Please check the merged commits, I have taken the liberty to already do those updates.

I just need to do the final update to the interface / procedure comment.

[Martin_fr]

OK, I was wrong on the whole timeout and thread stuff.

The thread is meant to, and must be by the caller. And the caller must have a means to enforce the timeout  (it currently has, but the API is somewhat not right).

The entire idea is that a debugger using this, should be able to run single threaded if it wants to. If it does, then it would poll for data, it would do that with a low (even zero) timeout, expecting just to know if there is or isn't data....

Albeit, it may not have to use this, but could then simply call read. But if the consumer then needs to sleep, a sleep with wakeup on data is preferable.

Only setting a thread object as the indicator, that a blocking check is allowed... That just doesn't feel right either.

Anyway, I will probably leave that for now.


----
There are other thread usages. And if a consumer should be able to omit "uses cthreads" then that may need review too.

[Martin_fr]

Some more thought process...

A blocking call to CheckForConsoleOutput does not work well.

If a non-threaded consumer wants to use that, it must know that it returns. So that this consumer can continue other work.

The next idea would be a different call "WaitForConsoleOutput" => which by default could just do the looping call to CheckForConsoleOutput.
But that has drawbacks too. If the call is blocking, then the consumer is forced to create its own thread, and call it from there. That means the provider (FpDebug) either shouldn't create a thread, or we end up with unnecessary 3 threads.

Further more, we don't know if a thread will be needed on all platforms.


But what should work is an event as alternative.

"alternative" because the original CheckForConsoleOutput continues as "no thread forced by FpDebug" if the consumer needs that. => whereas the event, would be "FpDebug may create a thread, if it desires to do so".

"alternative" also meaning: Either-or, but not both at the same time.

So if the event is assigned FpDebug creates the thread. And waits, and fires the event as needed.

Or, it may not create a thread, e.g. if it gets (or can get) the info in the "WaitForDebugEvent" loop. Then it would just fire the event (outside the "SendEvents" part)

So that would give full control to FpDebug.

It leaves a few decisions to be made.

- Start/stop => setting/unsettling the event? Or Explicit calls. (and property indicating if it is active)
- event fired in main thread, or the internal thread


The latter one would normally say main thread. And that is easy if (BIG IF) an internal thread is created.
If FpDebug knows it created an internal thread, then it can do QueueAsync or TThread.Queue.
But if it e.g. runs in the WaitForDebugEvent loop, then it may not know if it runs in a thread => and then at least with QueueAsync it would not know if to use it (TThread.Queue has detection if it is in main)


The other optional feature is, if (maybe a define) allows to prevent internal threads. Then an event may not be supported on every OS. But that might be a later extension.

[MattBradford]

I did the two checks you asked for before touching anything. The first one comes back clean; the second turned up something, and it is inside the function you named rather than in the callers.

Does any caller pass a name already taken from debug info?

No. Everything that reaches the DWARF by-name path carries text from outside the debug info: the expression parser, the nested-proc forwarders in fpdbgdwarffreepascal, the JSON converter's user-supplied function name, two hardcoded literals, and the exception-class cast, whose class name is read out of the target's VMT rather than out of DWARF.

The one place that does pass a debug-info name is TFpValueDwarfSubroutine.GetEntryPCAddress, which takes GetLinkageName and looks it up - but that goes to Context.SymbolTableInfo, so it never reaches CreateSubClass. And that side already does what you are proposing here: TFpSymbolInfo.FindNamedProcSymbol builds TFpSymbolTableProc from the name it got out of the symbol list, not from the requested one.

So on the evidence I have, no flag is needed for the reason you raised. That part is traced in source only - nothing built for it.

But the empty string is not free, and the case that pays is the unit

Three fpd builds off the same tree against one unchanged testee. A probe asks the scope directly and prints what the returned symbol carries. Declared spellings are GMixedCaseGlobal, UnitProc, and unit ProbeUnit in file probeunit.pas:

Code: [Select]

query               before            empty string      + NameNeeded
gmixedcaseglobal    gmixedcaseglobal  GMixedCaseGlobal  GMixedCaseGlobal
UnItPrOc            UnItPrOc          UnitProc          UnitProc
PROBEUNIT           PROBEUNIT         probeunit.pas     probeunit
ZzNoSuchNameHere    nil               nil               nil
GMixedCaseGlobal    GMixedCaseGlobal  GMixedCaseGlobal  GMixedCaseGlobal
Column two is your fix doing exactly what you said it would, in two different compilation units and on a variable and a procedure. The unit row is the problem: FindExportedSymbolInUnit returns the compile_unit DIE under fsfMatchUnitName, that DIE's DW_AT_name is the source file name, and TFpSymbolDwarfUnit has no NameNeeded of its own - so a unit symbol stops being able to name itself and reports the file instead. The last two rows are controls: a miss stays a miss, and a request that already matched the declaration is unchanged.

Column three is what I would suggest doing about it: give TFpSymbolDwarfUnit a NameNeeded that sets CompilationUnit.UnitName, then pass the empty string unconditionally. It is additive - CreateUnitSymbol already constructs those with a non-empty name, so NameNeeded never fires on that path - and it means a unit symbol names itself correctly however it was built, rather than one call site being repaired. It builds clean and the unit row goes green without moving any of the others.

One thing that column three does not fix, and I do not think can be fixed

The repaired name is probeunit, not ProbeUnit. UnitName is ExtractFileNameOnly of the compile_unit's DW_AT_name, so it carries the spelling of the file rather than of the unit clause - my file happens to be lower case and my unit is not. As far as I can see the declared spelling of a unit is not in this data at all, so the most the fix can promise is the file's basename. If you know somewhere else it could come from I would rather take that than ship the weaker guarantee.

That matters for the testsuite too: it makes the Sym.Name assertion two assertions, and the unit one is asserting a filename, which means the fixture's filename casing has to be chosen deliberately rather than by accident.

Two things I would leave out of the same MR unless you want them in. The FreePascal static class var path substitutes the requested name for a _static_<class>_<NAME> DIE, and there is no source-level spelling in that DIE to recover, so it has to keep what the caller passed. And there are six other places in TFpDwarfInfoSymbolScope that pass AName into CreateSubClass which I have not measured at all.

All of the above is win64, FPC 3.3.1, on the !757 merge commit, and I have not fetched since - so if main has moved under this I will re-run before filing anything.

Does the NameNeeded shape suit you, or would you rather the unit case were handled at the call site?

[Martin_fr]

Thanks.

Please test https://gitlab.com/martin_frb/lazarus/-/commit/b85b31d7a1a30361d421bbabbbce99703d932c93

I briefly thought about caching the name found in "GoNameChild" => but its an awful long way, and it can't be stored directly, because "NameNeeded" may need to process it. So not worth it.

-----
UnitName is indeed not available.

The filename can not be changed, because debuggers need it to look up the sources. (if they should show them).

I haven't checked if DWARF would offer an suitable additional field, which could be added in FPC.

[MattBradford]

Tested. b85b31d7a1 does what column three did, on all five cells, and it does it at a newer base than the one I measured on before.

The rows

Same probe and the same unchanged testee as in reply #24, so these columns compare with the ones there. Two fpd builds off one worktree detached at your commit, the control made by checking out 9a8bd674e2's copy of fpdbgdwarf.pas and nothing else, so the only delta between the two binaries is that file. Declared spellings GMixedCaseGlobal, UnitProc, and unit ProbeUnit in file probeunit.pas:

Code: [Select]

query               parent 9a8bd674e2  b85b31d7a1
gmixedcaseglobal    gmixedcaseglobal   GMixedCaseGlobal
UnItPrOc            UnItPrOc           UnitProc
PROBEUNIT           PROBEUNIT          probeunit
ZzNoSuchNameHere    nil                nil
GMixedCaseGlobal    GMixedCaseGlobal   GMixedCaseGlobal
Measured, ten runs, win64, FPC 3.3.1. The two exes differ, fpdbgdwarf.pas is named in both build logs, and the miss row still returns nil, so the probe is discriminating rather than printing whatever it last had.

What that does not cover, and one of the two is my fault for the testee I built

Your CompilationUnit = nil branch was never reached. Every symbol my probe gets back has a compilation unit, so the ReadName / ExtractFileNameOnly path is untested here and I would not describe the commit as measured in full.

Worse, and I only noticed writing this up: with your version the unit row no longer tells me which branch ran. CompilationUnit.UnitName is ExtractFileNameOnly of the compile_unit's DW_AT_name, and your fallback calls ExtractFileNameOnly on that same string, so both branches return probeunit here. My column three discriminated only by accident - its else fell through to inherited and gave probeunit.pas, which is a different string. So "CompilationUnit is non-nil on this path" is something I know from the earlier build, not from this one. If you want the fallback exercised properly it needs a case where CompilationUnit is nil, and I do not currently know how to construct one.

The base

I fetched before testing. main moved f1f988da49 -> 498bd29b97 while I was away from it. Your parent 9a8bd674e2 is in main, and the only commit after it that touches components/fpdebug is 498bd29b97 "Clean up, unused var ...", which does not touch fpdbgdwarf.pas - so for the file under test your branch tip and main are the same content. That discharges the caveat I ended reply #24 with.

UnitName

Understood, and I would rather have the weaker guarantee stated than a stronger one that is not true. It does mean the testsuite assertion on a unit symbol is asserting a filename, so the fixture's filename casing has to be picked deliberately - I will make that explicit in the test rather than let it pass by luck.

If DWARF does turn out to have somewhere FPC could put the declared spelling, I am happy to revisit it, but nothing in what I am reading suggests the information is there today.

Still unmeasured, unchanged from reply #24

The six other places in TFpDwarfInfoSymbolScope that pass AName into CreateSubClass, and the FreePascal static class var path, which substitutes the requested name for a _static_<class>_<NAME> DIE and has no source-level spelling in the DIE to recover.

What would you like next?

The commit is yours and on your branch, so I assume it goes in under your hand rather than through an MR from me - say if you would rather I filed one.

The part I can pick up either way is the testsuite. Reply #15 left the TestFpDebugApi fixtures waiting on which way question 1 went, and it has now gone.

One correction to my own plan there before I start, since you had already made the point and I never came back to it: you were right that ScopedEnums is not global. Your PUSH/POP example is per-declaration, so the scoped-enum collision does not need its own unit through TEST_USES after all, and all four cases fit inside WatchesScopePrg with no new source file at all. My "the one case that cannot go in there" in reply #15 was wrong. I will re-measure the fold in that shape rather than carry over the three-of-four result I got under the old assumption.

So: insertions into WatchesScopePrg only. I will start on those unless you would rather they waited until this is in main.

[Martin_fr]

sorry on the overlap. I decided to test the empty name in the testsuite. And then your post came in, and I just added the NameNeeded.

Yes the "if CU=nil" probably never runs. Well currently. But it doesn't cost.

Thanks for the big feedback, when I asked I only expected to get feedback if your debugger gets the correct name. That is the part I don't yet have a test for (neither testcase, nor running in a debugger / LazDebuggerFp does not call it).

next?

If you volunteer, I would be grateful for the test. As per the details mentioned.

Otherwise, you need to mention if anything on your wish list is still open.

[MattBradford]

Yes, I will write the test. One question first, because of the overlap you just mentioned.

What is already in the testsuite?

You said you decided to test the empty name in the testsuite. If you have already added a case for it, tell me what it covers and I will build around it rather than over it. You made this exact point on 24 August - "just checking, so we don't both do the same work" - and it applies more here than it did then, because we are now both looking at the same function.

If nothing has gone in yet, I will start from the shape agreed in reply #15 plus your two extra cases.

That the by-name path has no other exercise changes how I will write it

Your line that there is no testcase for it and that LazDebuggerFp does not call it is the useful thing in your reply, and I had not understood that. It means the temporary probe I have been quoting numbers from is currently the only thing that runs FindNamedProcSymbol at all, so the tests are not tidying up after an already-covered path - they are the first cover it gets.

Two consequences I will build in rather than discover later:

The Sym.Name assertion is two assertions, not one. An ordinary symbol asserts the DWARF spelling. A unit symbol asserts CU.UnitName, which is ExtractFileNameOnly of the compile_unit's DW_AT_name - the source file's basename, not the unit clause. So the fixture's filename casing is what that assertion pins, and I will choose it deliberately and say so in a comment, rather than let it encode an accident of how the file happened to be named.

The case-matching assertion has to be conditioned on the DWARF version, as you said in the G8 review, and the mangled-name assertion should construct the name with the debugger's own mangling code so it doubles as a regression detector on that.

On the fixtures themselves - reply #15 was wrong and I have now measured the correction

I conceded in my last post that your PUSH/POP counter-example refuted my "the one case that cannot go in there". I have since built it rather than just agreeing with it: WatchesScopePrg.pas with all four fixtures in one file, the scoped-enum type wrapped in PUSH/SCOPEDENUMS ON/POP, against an unmodified control build of the same file. Both compile clean with no warnings, and every existing unscoped enum reference in the main block still compiles - which was the whole of my objection.

So it is four fixtures, one file, no new source file and no new exe. Nothing existing moves.

I have not yet exercised the scoped-enum case through the by-name lookup - the probe I used for that run asks the generic symbol lookup rather than the proc path, so it does not answer the question the fixture exists to ask. That is one of the things the testcase will settle properly.

The fix itself

I assume b85b31d7a1 goes in under your hand rather than as an MR from me, since it is your commit on your branch. Say if you would rather I filed one.

What is still open on my side

Two things, both parked deliberately, neither needing anything from you today:

The address-0 readability policy moving into MemManager - the item I called D4. I am holding it until the fixes-5 branch is cut, because it should not be started against a moving target. When it branches I will re-raise the two questions from my reply #3 that are still open: whether it should cover the IsNilLoc shortcut in TFpValueDwarfPointer.GetDerefAddress, and the timing of the diagnostic-only part.

Worker-pool shutdown in a headless host. The only synchronous join in the tree is in finalization, so a consumer that does not call TerminateAllThreads(True) itself has workers alive into RTL shutdown. This is read from source only and it is not a diagnosis of anyone's crash - I am not filing it as a defect. Your aside in reply #4, that outside the IDE FpDebug may not need a worker thread, is the reason I mention it at all. If it is worth anything it is a question rather than an MR, and it can wait for the threading rework you sketched in #23.

Nothing else is open. The one-byte length bug in GetConsoleOutput that I reported on !709 in August is closed as far as I am concerned - your 6d51ba0b40 fixed both the length and the buffered-byte reset, and I have confirmed it in current main. Read, not measured: I traced it rather than re-running the case that produced it.

And to be explicit about the two items of yours that are not mine to chase: the interface-versus-implementation DWARF flag, and the event alternative you sketched in #23. I am not waiting on either.

[Martin_fr]

I have merged, and pushed that commit.


Testcase for FindNamedProcSymbol
Yours, I don't have plans to add them.

I decided to test the empty name in the testsuite.

I worded that badly: I decided to test the change in that commit (that set the name to empty), in the existing unmodified testsuite.

I.e. test the commit does not break the current test.

In case the code is reached by any other path / since I removed the name in all overloads of FindExportedSymbolInUnits

the temporary probe I have been quoting numbers from is currently the only thing that runs FindNamedProcSymbol at all

I though you had it added to your debugger already. But either way, you have code to call it, and get results. I don't.

The address-0 readability policy moving into MemManager

Details later (btw, I will be away in a week, for a week)

threads

Probably a new topic. I still have to decide some bits...

Generally important to make one distinction
FpDebug
LazDebuggerFp

* LazDebuggerFp
is currently not a concern of making it thread free. (albeit, there are defines for benchmarking, that will force it into single thread, but I don't know if they are useful for real debugger implementations)

And other consumers don't need to do threads. FPDebug should work fine if run in a single thread. DbgController.ProcessLoop can be called in the main thread (at least should be).
Of course it will then be blocking, when it does WaitForDebugEvent.

So, yes, a none threaded consumer will block there. That is by design. (that is how gdb also works, while running you can't send new commands / to interrupt you must signal the target app from outside the debugger / albeit gdb has an async mode)

I don't know if you were looking for something else on the above...

* FpDebug
provides TFpThreadWorkerQueue for
- outside usage (that is no concern, if not used, no problem)
- has some code that starts threads itself (finding symbols by name / hashing names / ...)

The latter of the "inside FpDebug" usages is what should be addressed. If the outer consumer doesn't do threads, then FpDebug also shouldn't use them internally.

Maybe have a factory for the queue? And pass a no-thread queue.

Worker-pool shutdown in a headless host.

Would depend on examples coming up.

Gut feeling: if you create a pool, you need to shut it down.
If you don't create one, it works as detailed in the Lazdebugger section. (but would want the fixes in FpDebug)