FpDebug: Bugs and API gaps, with proposed fixes

[MattBradford]

FpDebug: verified bugs and API gaps, with proposed fixes

Found while building a headless (non-IDE) consumer of components/fpdebug — an MCP debug server driving FpDebug over stdio JSON-RPC against Win64 and RISC-V embedded targets. Every source claim was re-verified against a current checkout of Lazarus main (line numbers from that tree, marked ~, may drift slightly against trunk). Items marked measured were also exercised against live targets. Posted for feedback before any MRs: the bugs come with patches, the gaps are proposals, and some may be by-design decisions worth defending instead.

Part 1 — bugs

D1. Pretty-printer access violation printing a :flatten() result

:flatten(Head^, Next) faults with EAccessViolation, while :len() of it,

• and [0..1] slices of it all work. Any single-member-key flatten faults; two or more keys do not. Cause: fppascalbuilder.pas, TFpPascalPrettyPrinter.DoGetArrayValue (~1320) probes one element past the end of the run to size a read cache:

Code: [Select]

repeat
  TmpVal := AValue.Member[StartIdx + min(CacheCnt, Cnt) + LowBnd];
  if IsTargetNotNil(TmpVal.Address) then begin   // <-- TmpVal may be nil
For a DWARF array, Member[] fabricates a value for an out-of-range index, so the probe is harmless. TFpValueFlatteArray members come from a list and out-of-range returns nil (fppascalparser.pas ~1774), so TmpVal.Address dereferences nil. MemberValue three lines up is nil-checked and the main print loop is nil-checked; only this probe is not. Multi-key flatten escapes because its elements are synthetic tuples with no target address, so the caching branch containing the probe is never entered. Secondary bug at the same site: on the IsTargetNotNil=False path, TmpVal.ReleaseReference is never called — the probe value leaks. Proposed patch:

Code: [Select]

TmpVal := AValue.Member[StartIdx + min(CacheCnt, Cnt) + LowBnd];
if TmpVal = nil then
  break;                       // probe past end: no caching
if IsTargetNotNil(TmpVal.Address) then begin
  ... existing body unchanged ...
end
else
  TmpVal.ReleaseReference;     // fixes the leak
break;
IDE reachability: the watch window goes through TFpWatchResultConvertor and never hits this path; breakpoint log expressions use PrintValue and should reproduce it (not tested in the IDE).

D2. PrintValue returns False for ddfPointer on a pointer — while producing correct output

Measured: ddfPointer on a PNode makes PrintValue return False ("invalid value") — yet ddfPointer is implemented. In fppascalbuilder.pas, DoPointer (~835), the ddfPointer branch ends in exit; // no data, which skips the Result := True at the end of the procedure, so failure is reported with a perfectly good string in APrintedValue. The class/interface ddfPointer branch in DoStructure (~1183) gets it right; only the skPointer branch forgets. Fix: insert Result := True; before that exit.

D3. Array slices ignore the array's declared bounds

Measured, with Arr: array[0..9] of Integer and DynArr of length 5:

Code: [Select]

Arr[8..20]    -> 13 elements, the last 11 are whatever follows the array
DynArr[0..99] -> 100 elements, 95 of them heap garbage
No error, no truncation. In-range slices are correct, which makes the out-of-range output dangerously plausible — an automated consumer has no second signal to cross-check against. Cause: TFpPasParserValueSlicedArray.GetMemberCount (fppascalparser.pas ~7766) is EndValue - StartValue + 1 — the count comes entirely from the literals in the expression — and GetMember re-evaluates the base expression per index with no bounds check either.

The clamp mechanism already exists: EndValue (~8014) honours an FHasUpperLimit/FUpperLimit pair, and the slice controller (~7887) populates it from the base value — but only when vfArrayUpperBoundLimit is set, and the only place in the tree that sets that flag is TFpValueFlatteArray.Create. Flatten results are protected; ordinary DWARF arrays are not. Proposed fix, in the same controller block: when the base is skArray with a known high bound, feed the same pair:

Code: [Select]

else
if (tmp.Kind = skArray) and (tmp.IndexTypeCount > 0) and
   tmp.IndexType[0].GetValueHighBound(tmp, hb)
then begin
  FSlicePart.FUpperLimit := hb;
  FSlicePart.FHasUpperLimit := True;
end;
plus Max(0, ...) in GetMemberCount for inverted/empty ranges, and a matching low-bound clamp in StartValue. Where no bound is known (open or pointer-backed arrays) the current permissive behaviour remains — as the documented exception rather than the default.

D4. A pointer whose value is 0 cannot be dereferenced ("Internal dereference error"), and address 0 is hard-coded unreadable

This matters most on bare metal: on the WCH CH32V parts I debug, 0x0 is the flash alias — the reset vector and initial SP live there. It is the first address a bring-up session wants to read, and the one address FpDebug will not touch. Measured on CH32V103 over a remote link — nothing below the evaluator refuses address 0:

Code: [Select]

read_memory 0x0          -> 6f2080066f20000d...  (reset vector; identical at 0x8000000)
evaluate PByte(0)        -> "Internal dereference error"
evaluate PByte(1)        -> "PByte($00000001)"    fine
evaluate PByte($8000000) -> "PByte($08000000)"    fine
Address 1 is no more "valid" than 0 by any general rule, and it is accepted — whatever refuses 0 tests for zero specifically. Traced: TFpValueDwarfPointer.GetDerefAddress (fpdbgdwarf.pas ~2795) initialises Result := InvalidLoc and skips the address read when IsNilLoc(OrdOrDataAddr). For the typecast constant PByte(0), OrdOrDataAddr is ConstLoc(0); IsNilLoc (which includes mlfConstant at address 0) is True; the deref address stays InvalidLoc, and GetMember (~3093) then hits if not IsTargetAddr(addr) and reports "Internal dereference error". PByte(1) passes because ReadAddress resolves ConstLoc(1) to TargetLoc(1). The pretty-printer compounds it: DoPointer prints "nil" correctly, but its speculative PChar probe (Member[0]) sets the error above, and the tail of InternalPrintValue replaces the successful output with the error string.

The policy layer: four predicates in fpdmemorytools.pas conflate "this pointer is nil" with "this address cannot be read" (IsReadableMem, IsReadableLoc, IsNilLoc, IsTargetNotNil all test Address <> 0). The first two are virtual on TFpDbgMemModel — and TFpDbgAvrMemModel already overrides both with no zero test, because AVR has real memory at 0. So "a target whose 0 is real can opt out" is settled policy; the problems are that many call sites use the free functions directly and never consult the model, and the deref path above has its own IsNilLoc shortcut that bypasses both.

Proposed, in increasing ambition: (1) fix the diagnostic — set a real "cannot dereference nil" error in GetDerefAddress, and have DoPointer clear the value's error when the speculative probe fails so it cannot clobber a successful print; (2) drop the IsNilLoc special case and let ReadAddress resolve the constant — hosted targets then fail honestly with "cannot read memory at address 0", targets whose model permits 0 succeed (needs a survey of GetDerefAddress callers first); (3) route the remaining zero tests through the model so the AVR-style override works everywhere. Item 1 is worth doing regardless of 2 and 3.

Part 2 — API gaps

Things a headless consumer needs and cannot get, ordered roughly by smallness times payoff.

G1. TFpDbgBreakpoint has SetCondition but no getter. Interface at fpdbgclasses.pp ~580: no GetCondition, no property, though FCondition already exists on the base class. A consumer that reports breakpoints back to its user must carry a parallel map. Ask: a one-line read accessor.

G2. A breakpoint condition has no error channel and is never validated. IsValidHit (~4466) starts Result := True and only overwrites it when the expression is both valid and boolean — a typo'd or non-boolean condition stops on every hit exactly as if it were absent, with no error text, flag or event (measured). The author's own TODO ("parse expression when breakpoint is created") sits directly above. Ask: validate at SetCondition, and/or expose a ConditionError filled when evaluation falls back to True.

G3. No hit/ignore count, and no way to add one from outside. AddBreak (~2641) hard-codes TFpInternalBreakpoint.Create — no class-of hook, no virtual factory — so a consumer cannot subclass IsValidHit to count hits; counting in the consumer's stop handler costs a real halt per hit, which on a slow debug link is the expensive path conditions exist to avoid. Ask: HitCount/IgnoreCount on the base honoured by IsValidHit, or a breakpoint-class hook on TDbgProcess (the smaller change, and it unblocks more).

G4. SetCurrentThreadId fails silently on an unknown id. fpdbgcontroller.pas ~1681: unknown id → debugln and exit, with the raise commented out. Ask for thread 99, keep thread 1: every subsequent register read and backtrace answers about thread 1 while looking like thread 99. Ask: TrySetCurrentThreadId: Boolean (or honour the raise), keeping the property as the convenience form. Same visit: GetCurrentThreadId (~1650) has no nil guard (AV before the process exists), and GetDefaultContext (~1663) caches a context that SetCurrentThreadId does not invalidate — between a thread switch and the next ProcessLoop iteration, DefaultContext describes the old thread; a ReleaseRefAndNil in the setter closes it.

G5. Out-of-range stack frame surfaces as "identifier not found". FindSymbolScope (~2904) never returns nil for a bad frame: it falls through to a TFpDbgSymbolScope over a zero-address context, so a wrong frame number reads like a misspelled variable. There is also no way to get a frame count without paying for the unwind and measuring CallStackEntryList.Count. Ask: return nil (or a distinct error) for an out-of-range frame, or expose the count.

G6. DW_AT_artificial is parsed, cached, and unreachable. TFpSymbolDwarf.IsArtificial (fpdbgdwarf.pas ~4866) reads and caches the attribute; the DWARF plumbing below it is complete. But it is protected, and TDbgSymbolFlag has no member for it, so a consumer holding a TFpSymbol cannot ask. "List locals" features must therefore guess from names: a $-prefix rule catches $result and $parentfp but misses self (plain name, correctly artificial in DWARF — FpDebug itself checks IsArtificial when hunting the self parameter), and class members _vptr$TOBJECT and _MonitorData are indistinguishable from fields a programmer happened to name that way. Ask: add sfArtificial to TDbgSymbolFlag, set from the existing cached read — one enum member and one assignment — or promote IsArtificial to a public virtual on TFpSymbol.

G7. A watchpoint that failed to install is indistinguishable from one that works. AddWatch (~2683) returns a live object unconditionally; the constructor ends in SetBreak, a procedure whose only failure handling is to undo itself. And the caller cannot ask afterwards: TFpInternalWatchpoint descends from TFpDbgBreakpointBase, so its State is the base GetState — bksUnknown, always — while bksFailed sits unused in the enum for this class. Two paths reach it: x86 debug-register exhaustion (a wide or unaligned watch consumes several Dr slots), and the base TFpWatchPointData.AddOwnedWatchpoint, which is unconditionally False — a backend that does not override it fails every watchpoint, silently, forever. Measured end-to-end: four watchpoints install, the fifth "succeeds" and never fires; the only recourse is scanning TFpIntelWatchPointData.Owner[0..3] by pointer identity, which is Intel-only. Ask (any one closes it): make SetBreak return Boolean or set a failed flag; give TFpInternalWatchpoint a GetState override returning bksFailed; or expose SlotsTotal/SlotsUsed virtuals on TFpWatchPointData so refusals can say why.

G8. No DWARF name-to-address lookup for source-level function names. TDbgInfo.FindProcSymbol(AName) returns nil and TFpDwarfInfo does not override the by-name overload (verified), so resolution falls through to the PE/COFF table of FPC-mangled linker names. The expression evaluator's scope resolves @Add fine, so the information exists; the by-name entry point just does not reach it, and every consumer of AddBreak(AName) reimplements the fallback. Ask: implement the by-name FindProcSymbol in TFpDwarfInfo.

G9. Member[] out of range: two contracts. DWARF arrays fabricate a value for out-of-range indices; list-backed values (flatten) return nil. Both defensible, both undocumented; the combination is what turned the D1 probe into an AV — the printer nil-checks one of its two probes but not the other. Ask: document the contract, and make it uniform.

G10. GetInstanceClassName can leave an error on a healthy value. LazDebuggerFp knows the folklore: ResValue.ResetError; // in case GetInstanceClassName did set an error (fpdebugdebuggerworkthreads.pas ~1258). A consumer without that line gets a value reporting an error it does not have. Ask: do not set the error for the no-RTTI case, or document the required reset.

G11. Three display formats declared but silently ignored. ddfChar, ddfString and ddfBinary have no implementation anywhere — the int and cardinal branches carry "// TODO ddfChar:" and fall to the default — so the caller gets a plausible number in the wrong base with no indication (measured: byte-identical to ddfDefault across seven types). ddfPointer is D2 above. Ask: implement them, or have PrintValue return False for a format it will not honour, so callers can tell "value in binary" from "request ignored".

G12. Backend failures flatten to fpErrAnyError — no neutral target/link failure classes. The error machinery is better than its usage suggests: TFpErrorCode is an open Integer space in informal bands, TFpError chains (the top link stays branchable via ErrorCode() while inner links keep their own codes), and OnErrorTextLookup lets codes FpDebug has never heard of still render. But the code list is entirely evaluator- and hosted-process-shaped, so an embedded backend has nothing neutral to map to: link lost, reset failed, target read-protected, hardware slots exhausted all become CreateError(fpErrAnyError, ['text']) — code 1 plus prose, unbranchable (measured in an out-of-tree RISC-V embedded backend). Ask: a small neutral band, chosen by what a generic caller would act on differently, not by cause:

Code: [Select]

// 20000 target / debug-link errors
fpErrTargetConnectionLost    = TFpErrorCode(20000);  // re-attach / replug
fpErrTargetBusy              = TFpErrorCode(20001);  // retry after halt
fpErrTargetTimeout           = TFpErrorCode(20002);  // retry or abort
fpErrTargetAccessDenied      = TFpErrorCode(20003);  // protection; needs user action
fpErrTargetResourceExhausted = TFpErrorCode(20004);  // hw slots (data: used, total)
fpErrTargetNotSupported      = TFpErrorCode(20005);  // disable the feature
Admission test: if no consumer would take a different action on it, it is message text, not a code. Backend-specific detail (DMI cmderr values, protocol errors) stays in a private band and rides as the inner chain link. Related: enums like TFpDbgBreakpointState should stay coarse (they drive state machines; every added member grows every case in every consumer) and instead gain a LastError: TFpError — the same accessor G7 and G2 want; the real information sinks are the Booleans (SetBreak, AddOwnedWatchpoint, ReadData) that discard the reason. Two housekeeping notes: OnErrorTextLookup is a single function pointer, so two providers must chain it manually — a registration list would fix that; and the code bands are convention only — a documented reserved range for out-of-tree backends would avoid collisions.

Suggested order. One-liners first: G1, D2, G6, G4. Then the confidently-wrong-answer class: G7, G2, G5, D3 (the clamp machinery already exists). Then D1, D4 item 1 (the diagnostic half), G11, G3, G10, G8, and finally D4 items 2 and 3, which want a maintainer's view on blast radius. G12 is a design conversation rather than a patch, but it is the natural home for the LastError accessors G7 and G2 want, so it is worth raising alongside them.

[Martin_fr]

D3. Array slices ignore the array's declared bounds

Its intentionally allowed. The user can choose here.

E.g. there is a common pattern of an array as last record member.

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

1. record
2.   fieldfoo: TType;
3.   data: array[0..0] of TData;
4. end;

The array actually has variable size, the record is accessed via pointer, and mem can be allocated.

Sometime the array also has 0..high(type) bounds.

I have to check, but I would expect some access to High/LowBound is available?

Albeit some of those classes need rework, because currently some stuff needs to cast to the Dwarf subclasses, and it should all be possible via the 3 baseclasses (TFpSymbol either TFpDataSymbol or TFpTypeSymbol) and mostly TFpValue.

A pointer whose value is 0 cannot be dereferenced

Indeed, and actually known (Thanks to Christo who does the avr stuff).

FpDebug is still heavily Intel bound. I have now started Aarch64. So far I have "patched it on" only. I don't want to do a big rework, as the fixes-5 is likely soon branched (and I don't want to do dangerous changes right before that).

Once that happened, its a matter of spare time.

As for "some are virtual" => Targets should not need to override entire methods, because the base has one wrong check. The IsNil check should probably go to MemManager. So one place were that can be controlled.
I don't want to put all the other checks there because some are equal for all targets, and then there is no need to trigger a virtual call.

Semi-related: "internal deref error" => not the desired error message. But, so far my priority was to get the functionality right. Error message are still somewhere on the list.


Note, the entire internal breakpoint will probably need some work... I want to be able to do more inside the thread (because thread switches are time consuming, and if breakpoint should just continue....)

G1. TFpDbgBreakpoint has SetCondition but no getter

That can probably be changed quickly.

G2. A breakpoint condition has no error channel and is never validated.

Indented.

It can't be validated. Except in some cases.

A breakpoint may have several addresses. Even if set on a pascal line.
- Multi line function calls return several times to the first line. Currently there is no detection what is actually the begin of the statement.
- More important, in generics, each specialization has a breakpoint. And the variable may have a different data type in each of them.
- breakpoints may be pending for a LoadLibrary (or change if Unload followed by another Load)
- Watchpoints can stop just anywhere

So at least for the 2nd case there is on way. In the 3rd case, validation may be possible, but the result may be outdated by the time the breakpoint is reached.

And in case there is just one address, you still need to create a context for being able to test the condition. The breakpoint is often set, while not paused in that function, so you need to build a virtual frame for local vars....

Parsing could be tested. But for that you don't need the breakpoint. You need an API that does it directly.

Then a debugger/IDE may trigger checks before as it sees fit. I.e. call the syntax check. Or if it knows its paused at the desired frame, then run a watch eval to see if there is a result => it then needs to filter out soft errors (e.g. nil deref).

G3. No hit/ignore count, and no way to add one from outside.

If anything then a hook/callback.

Some of that will be needed in the IDE too. As currently not only hitcounts, but also enable/disable other do need the switch to the IDE main thread.

I haven't decided on details yet.
But I am pretty sure the class will not become inheritable.

The original design was that the debugger would have its own classes, and the breakpoints in the FpDebug are like a handle.

G4. SetCurrentThreadId fails silently on an unknown id.

Adding an exception will need tons of code to be checked...

Yes, it shouldn't be reached. But, while currently a debugger may not behave as expected, the debug session can be continued. An exception could kill the debug session of a debugger (if that debugger did the wrong thing) and that could destroy an hour of work getting the debugged app into the wanted state...

I also have to check how I handle that with other properties (e.g. a stackframe that isn't avail)

Will take some time to investigate.

G5. Out-of-range stack frame surfaces as "identifier not found".

I think that is an error on the caller site. (I haven't traced all possible calls to it though). This should only be called with a valid thread/stack.

The caller must give the frame for which it wants that scope. If that frame does not exist, then the caller must not use it.

If the caller gets the frame-id from an untrusted source then it must itself check against the stack. Yes that means unwind, but FindSymbolScope would do the unwind too.

If you have any example were that is not happening, or not possible ....

G6. DW_AT_artificial is parsed, cached, and unreachable.

Generally no problem with adding a flag. Not necessary a 1 to 1 mapping with DWARF, since the base classes are an abstraction, and other format could be added in future.

sfArtifical would still be a good name. But its documented as "created by the compiler / not declared in source). However that may be detected.

Not sure if _MonitorData  has that attribute, that is a normal field. It in the Pascal source of TObject.

Mind that maybe to some extend the aliases (eg. for result) could be linked. Don't know yet.

_MonitorData

I have to check / best report as a bug.

I thought the IDE shows them as disabled. (there was some old code, that prevented the IDE from using the invalid state / that needs to be fixed => when I work on moving to the new API)

But maybe that was on GDB... Also the mapping to disabled would be in FpDebugDebugger.

But in general, I need to very carefully check if it currently can use its state... And it may be that this will only be fixed when I move forward with the new API (between front/backend). It may be that this only limits FpDebugDebugger.

G8. No DWARF name-to-address lookup for source-level function names

If so (not checked right now) then fine with adding that.

Will need to see where to best add.

G9. Member[] out of range: two contracts.

Out of range calls are deliberately allowed.

Documentation... Unfortunately for now way down low on the todo list.

Also, as mentioned before, the contract is not complete (and may need shaping). IIRC there are needs to cast to the Dwarf subclasses, and that should not happen (outside the dwarf units).

There are also some issues with getting data from symbols (data or type symbols) that should only be on values. So the contract will still get breaking changes. But low on the todo, so not sure when.

Not sure where, I recently explained those 3 classes somewhere.

G10. GetInstanceClassName can leave an error on a healthy value

Not checked in detail, but the method is called on the value, so it looks like the error would be expected there.

ResValue is a TFpValue, and that is the place to hold the error.

This is the same as calling GetMember on a ResValue => I don't know if it currently would ever set an error, but by design I think it should be allowed. After all if it returns nil, then that is the only place.

Overall, error handling is far from what it should be. Some errors are replaced with more generic ones, others are passed through but may not have enough context added... This is a huge project to get it working well.

As before, documentation is very low on the list currently.

G11. Three display formats declared but silently ignored.

PascalBuilder isn't currently used in the Lazarus IDE, so yes, it had no active maintenance in a while.

Happy to accept an MR for either solution.

G12. Backend failures flatten to fpErrAnyError — no neutral target/link failure classes.

See comments on error handling above.

[Martin_fr]

Pushed fixes for the D1,D2 items.

One-liners first: G1, D2, G6, G4.

D3 is by design.

D4 - We can talk about an MR (moving check to MemManager).
Though
- not sure if it is accessible everywhere.
- May need to see, if the check is needed everywhere, to avoid repetition. E.g. knowing if the caller/callee did the check already.

G4 - not sure.

G6 - happy for an MR.

[MattBradford]

Thanks for the quick and thorough response — and for pushing the D1/D2 fixes the same day. I'll pull main and re-test both against my repro cases.

The by-design items are all understood and I'm dropping them from my list. D3: the array[0..0] variable-length trailing-member pattern is a use case I hadn't weighed, and it settles it — I'll clamp on my side using the declared bounds where they exist and leave the escape hatch alone. G5: agreed it's the caller's contract; I already prepare-and-check and have no case where that isn't possible. G2: the generics point (each specialization its own instance, same variable different type) is decisive — for what it's worth, parse-only checking at set time plus re-evaluating at the actual stop is exactly what my server does, so no ask remains there. G9/G10: understood, I'll keep the ResetError and treat out-of-range Member[] as permitted.

MRs I'd like to take, smallest first. G6 (sfArtificial): set from the existing cached read, documented as "compiler generated / not declared in source" rather than anything DWARF-specific — and agreed on _MonitorData, it's a declared field in TObject's source, so it wouldn't be marked; my concern was only that the flag be reachable at all, not what FPC hydrates it with. Then G11: my inclination is to implement ddfChar/ddfString/ddfBinary where they're meaningful and return False for a format the printer won't honour, but if you'd rather keep the surface small I'm equally happy with the return-False-only version — say which and I'll shape it that way. Then G8, where I'd appreciate a pointer on where you'd like the by-name lookup to live before I start.

D4: agreed on waiting for the fixes-5 branch, and MemManager as the single place for the policy sounds right. One question before I shape that MR: TFpValueDwarfPointer.GetDerefAddress has its own IsNilLoc(OrdOrDataAddr) shortcut that skips the read entirely, so that path would never reach a MemManager check — should the MR route it through the same check, or do you want pointer-nil semantics kept separate from address-readability there? Separately, the two diagnostic-only pieces (a real "cannot dereference nil" message instead of "internal dereference error", and not letting DoPointer's speculative PChar probe clobber an otherwise successful print) look branch-safe to me — happy to do those pre- or post-branch, your call.

G3/G4: no push from me — the hook direction and breakpoints-as-handles both work for my use, and the session-killing concern on G4 is fair; my server validates ids before handing them over anyway. I'll re-raise the watchpoint-install-status question (G7) when the breakpoint internals rework lands, since it lives in the same neighbourhood.

[Martin_fr]

As indicated, happy with any MR on PascalBuilder.

Same for artificial flag as bespoken.


G8:
I hadn't had a close look the first run. (the IDE always evals a watch, so the issue hadn't come up yet).
On 2nd glance it looks it just neeeds TFpDwarfInfo to override the virtual base function? So that is ok.
- Just please check if it get currently called at all, and what behaviour might potentially be affected.
- Btw, did you try to run the testcase? (in LazDebuggerFp)
  You need the fpclist.txt, see sample file / create a logs dir, create a lib dir in testapps, and on Linux create symlink TestApps => testapps



G1 also happy to have getter and property on the interface

G3 you may be able to deal with that in the FDbgController.OnThreadProcessLoopCycleEvent event. But not sure if that scopes with multiple breakpoints.
Happy to have an  On_Thread_CheckHit event / IMHO run after the condition passes?

----------------------
Outside the IDE you may not need to run it in a thread. Unless you need to keep other code active (i.e. a user console interface or a server connection).
According to some comments, there are some OS where only the thread that started the process can access it (memread etc). At least on Windows that does not actually matter.  But should be kept (there is a define to run asserts / so it can be tested )

[MattBradford]

Martin,

Picking up the by-name `FindProcSymbol` item. Both preconditions from your
earlier reply are done, and the second one changed my mind about what the ask
should be — so this is a question rather than an MR.

**What is there now.** `TDbgInfo.FindProcSymbol(AName)` is virtual and returns
nil; `TFpDwarfInfo` overrides only the address overload. `TDbgInstance.
FindProcSymbol(AName)` (`fpdbgclasses.pp:2496`) asks `FDbgInfo` first and falls
back to `SymbolTableInfo`, so a by-name lookup only ever meets the PE/COFF
table of mangled linker names. A source-level Pascal name does not resolve
through that path at all.

**Precondition 1 — is the by-name overload called? Yes, and that is the
problem.** Every in-tree by-name caller I can find passes a mangled or linker
name, and works *because* of that fall-through:

- `fpdebugdebugger.pas:2910-2952` — the internal breakpoints (`FPC_BREAK_ERROR`,
  `FPC_RUNERROR`, `FPC_RAISEEXCEPTION`, `FPC_RERAISE`, `FPC_POPADDRSTACK`,
  `FPC_CATCHES`, `__FPC_except_handler`, `__FPC_finally_handler`, `_FPC_leave`,
  `__FPC_specific_handler`, and `RtlRestoreContext` / `RtlUnwindEx` in a lib),
  via `AddBreak(AFuncName)` and `TFpInternalBreakpointAtSymbol.Create`
- `fpdebugdebuggerbase.pas:111` `GetCached_FPC_Func_Addr` — the four
  `FPC_*STR_DECR_REF` / `SETLENGTH` helpers
- `fpdebugvalueconvertors.pas:436` and `:535` —
  `SYSTEM_$$_GETVARIANTMANAGER$TVARIANTMANAGER` and `sysvartolstr`

Since `TDbgInstance` consults `DbgInfo` first, simply implementing the by-name
override on `TFpDwarfInfo` would change what every one of those resolves to,
so that is not a change I would make blind.

**Precondition 2 — the LazDebuggerFp testcase runs.** fpc 3.3.1, x86_64-win64,
`symbols=gw3`: 35 tests, 0 errors, 3 failures on unmodified `main`
(`TTestWatches` 1 — `TestWatchesValue`, 360 of 196238, all `class const` reads;
`TTestStepping` 2 — `TestStepOver` and `TestStepOver_NextOnlyFalse`, 2 of 91
each; `TTestBreakPoint` 8/8 clean). That is the baseline any patch gets
compared against. Two local build tweaks were needed and are not part of any
patch: this box has no `windres.exe`, so `{$R sources.rc}` became
`{$R sources.res}` as the comment there suggests, and I built the console
runner (`NOGUI`) for scripting, which needs `TestDbgControlForm` left out of
the uses clause so the `TestDbgControl` hooks stay nil.

**Now the part that changed my mind.** I had assumed the missing by-name
lookup left consumers stuck. It does not, and I would rather show you the
measurement than argue it. Trivial DWARF-3 target, fpc 3.3.1 x86_64-win64,
driven with `fpd` built from this same tree:

- `break MarkerProc` — `AddBreak` returns non-nil and fpd reports "Breakpoint 1
  added", because `TFpInternalBreakpointAtSymbol` is constructed regardless.
  The address list is empty and the program runs to completion without
  stopping. The by-name breakpoint silently does nothing.
- `break P$TESTEE_$$_MARKERPROC` — stops, at `$0000000100001670`.
- At that same stop, `eval @MarkerProc` gives `^procedure($0000000100001670)`,
  the same address; `eval @Add` gives `^function($0000000100001640)`.

So the name-to-address information is not merely present in principle — the
expression scope hands it over at the exact moment the breakpoint would be set.
And `break` refuses before `run` ("No Process"), so there is no window in which
`AddBreak(AFuncName)` is callable but a working scope is not:
`TDbgProcess.FindSymbolScope(AThreadId, 0)` returns a real DWARF scope whenever
there is a live process, and `DbgInfo` and `MemManager` are public if a caller
wants to build a location-less one directly (`FindSymbolScope(Ctx, 0)` lands on
`CreateScopeForSymbol(..., nil, ...)`, whose `FindSymbol` goes straight to
`FindExportedSymbolInUnits`).

There is also no in-tree caller that would use a new lookup.
`TFpDbgValueConverterJsonForDebug.GetProcAddr` tries the scope first and then
`FindProcSymbol` — I had expected that to be a workaround for the missing
lookup, and it is not; it is two namespaces queried on purpose.

**So the ask reduces to this**, and it is smaller than I set out with:
`AddBreak(AFuncName)` and `FindProcSymbol(AName)` take a name in one namespace
while the expression evaluator takes the other, and the by-name breakpoint
fails *silently* when handed the wrong one. A non-nil pending breakpoint that
can never bind is indistinguishable from one that will. To me the silent half
is the worse half, and it may be the more useful thing to fix — possibly
without touching name resolution at all.

**If you do think the lookup itself is worth adding**, the shape I had in mind
was an optional search-order parameter on the by-name family, defaulting to
current behaviour so nothing existing moves. Four things I would want your
answer on before writing it:

1. Boolean or enum? The declaration block already uses defaulted booleans
   (`AIgnoreCase`, `IsFullLibName`), so a boolean is in keeping — but there are
   arguably three orders worth naming, which is an enum's job.
2. Which overloads carry it? `TDbgInstance.FindProcSymbol(AName, AIgnoreCase)`
   plus `TDbgProcess`'s `(AName)`, `(AName, ASymInstance)` and the
   list-returning `(AName, ASymInstance, ASymList, AIgnoreCase)`. I would leave
   the two already marked deprecated alone.
3. Should `AddBreak(AFuncName, ...)` carry it through?
4. `TDbgProcess` descends from `TDbgInstance`, so it already carries the
   inherited `(AName; AIgnoreCase: Boolean = False)` alongside its own
   overloads. Another defaulted parameter is where overload resolution could
   quietly re-bind an existing call. I would compile-check that rather than
   assume it, but if you would rather a differently-named entry point for that
   reason, say so.

And if you would rather it stayed out of `FindProcSymbol` altogether — your
TODO at `fpdbgdwarffreepascal.pas:2721` suggests the CU is where the lookup
wants to live — I would rather hear that now than after a patch.

One small unrelated thing found on the way: `TDbgInstance.FindProcSymbol`
(`:2500`) passes `AName` to `FDbgInfo.FindProcSymbol` without `AIgnoreCase`,
so the flag only ever reaches the symbol table. Intentional, or worth fixing
while the file is open?

[Martin_fr]

On the testcase

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

1. [fpc64 trunk]
2. exe=C:\FPC\fpc_3.3.1\64\gw\bin\x86_64-win64\fpc.exe
3. symbols=gw,gwset,gw3,gw4
4. version=030301
5. bits=64
6.  

Add the version, and it should pass.

FPC removed the debug info for class const. Because gdb did fail on it, and since now TObject carries a class const, that failed all objects under gdb.

Technically, the old FPC debug info wasn't following exactly the DWARF spec. There is a correct way, but GDB does not like it either... Their should be an open report or MR on this, somewhere...

[PascalDragon]

Picking up the by-name `FindProcSymbol` item. Both preconditions from your
earlier reply are done, and the second one changed my mind about what the ask
should be — so this is a question rather than an MR.

Please use the forum's BBCodes instead of Markdown.

[Martin_fr]

Lets start with just technical notes and info, before going to decisions.


Most of the current callers should be fine, and also should be part of the testsuite.

- FPC_ => 1) tested. 2) if those existed in dwarf they should be the same (but test would need to confirm)
- Rtl.... (unwind/context) those are kernel. they should not be in dwarf ever. They should be called with the correct "LIB" instance. Otherwise they would already be at risk of hitting user named procs.
- ...$$... should be on the safe side

=> the "converter" is the one that would really need testing....
Also given that it already checks dwarf first.... But yes, would need testing.

`AddBreak` returns non-nil and fpd reports "Breakpoint 1  added"

Breakpoints are always created. Because they could later become valid, when libraries are loaded.

Only break that fails (but still afaik gives an object) is on a file/line: if the file exists, but the line is outside. Or if the break is set ONLY for one library.

In general it would be nice if breakpoints would accept all kind of routine names.
If they are made to do that, then the issue of "breaking existing" comes up, never mind if the "FindProcSymbol" is extended, or if the use a context. In that case the breakpoint themself would need to also accept some flag what namespaces to search.

So the name-to-address information is not merely present in principle — the
expression scope hands it over

While probably currently not the case for procs, scopes have a different search order (and scope).

A Scope would (if possible / currently not) first return nested procs, relative to where the RIP of the scope is.
A Scope will then search the current unit, and then other units (that isn't a problem).

A none scope search just searches all units.

But right now, that doesn't matter. Should be the same (the order in the SymList may differ, but that really should not matter).

There is also no in-tree caller that would use a new lookup.
`TFpDbgValueConverterJsonForDebug.GetProcAddr` tries the scope first and then
`FindProcSymbol` — I had expected that to be a workaround for the missing
lookup, and it is not; it is two namespaces queried on purpose.

Not sure, it may have been a workaround.

And if you would rather it stayed out of `FindProcSymbol` altogether — your
TODO at `fpdbgdwarffreepascal.pas:2721` suggests the CU is where the lookup
wants to live — I would rather hear that now than after a patch.

That comment is for another reason: The "$fin" proc that this looks for must be from the same CU.
So now after getting the proc, we compare that it has the same CU.

That is because a finally block is (in Pascal) part of the same procedure as the "try" block (and the code outside it). But in the exe, finally is a procedure. (with $fin in the name). So we try to find that procedure, and because it code is in the same procedure/same unit, the result must be in the same CU.

[Martin_fr]

Actually: Scope.FindSymbol

whose `FindSymbol` goes straight to
`FindExportedSymbolInUnits`).

If you give it a unitname. Otherwise it starts in locals of the current routine, then field of the class....

That means it can find a variable of the same name, before finding a proc.

It also can find a global var or type of that name...

So it different.


On the actual what to do.

I generally think extending FindProcSymbol is a good idea. It always was the idea anyway.

And there is even one more... Dwarf can in addition to the readable names contain linker-names too. And those may even be different than the ones currently searched. They aren't currently processed, and can't currently be searched. But nothing says that this may not change one day.

So an extra argument for the search scope would be a good idea, and it would be an enum, or set (scope, not order)

A set would be nice, as it extends well if more lookup types come up.

Imho the order is always
1) DWARF, readable names
2) DWARF, linker name (in future)
3) symbol tables

That order is fine, if you can select any subset of them.
Maybe "TFpProcSearchFlags: set of (psfDwarfName, psfLinkerSym)"

Order does not matter, if you get a SINGLE result, you can make several calls with each only a single scope search. Then you can order the calls.
If you get a list, then you can order the list if need. And also if need the returned symbols can carry a property "ProcNameFlag" that repeats the value that applied. (but I wouldn't add it, until it really is needed)

Optional, new overloads (old to be deprecated) and AIgnoreCase becomes a member in the set: psfIgnoreCase.
Easier to read than a "True" on its own.


Btw, as its a set, it can be empty (or only flags that don't give any of the scopes) => that shall mean "default" and see below, then that shall be "all scopes"

Which overloads carry it?

All please, except the deprecated one.

And also the AddBreak, yes please. (and if the IgnoreCase goes into the enum/set, then that can be changed here too - overload and old deprecated).

`TDbgProcess` descends from `TDbgInstance`, so it already carries the
   inherited `(AName; AIgnoreCase: Boolean = False)` alongside its own
   overloads. Another defaulted parameter is where overload resolution could
   quietly re-bind an existing call.

Good point. The existing could all be changed to have the correct defaults. But that only works if the new default for the enum/set is "[psfLinkerSym]" => so the old and new all have the same behaviour.

But, IMHO its much more desirable to change that default and change all (or change only dwarf names? but IMHO all).
Well, if you have any concerns about that, let me know

But in that case the name needs to change. And, that will also make them easier to identify in code (search/replace).


So the new ones on TDbgProcess/Instance (if enum deprecates the old ones, and if ignore case goes into the enum) would look like

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

1. const DEF = TFpProcSearchFlags([]);
2.  
3. // On TDbgInstance (does not take an instance / only searches this instance)
4.     function  FindNamedProcSymbol(const AName: String;  AProcFindFlags: TFpProcSearchFlags = DEF): TFpSymbol; overload;
5.     procedure FindNamedProcSymbol(const AName: String; out ASymList: TFpSymbolArray; AProcFindFlags: TFpProcSearchFlags = DEF);
6.  
7. // On TDbgProcess
8.     function  FindNamedProcSymbol(const AName: String; ASymInstance: TDbgInstance = nil; AProcFindFlags: TFpProcSearchFlags = DEF): TFpSymbol; overload;
9.     procedure FindNamedProcSymbol(const AName: String; ASymInstance: TDbgInstance = nil; out ASymList: TFpSymbolArray; AProcFindFlags: TFpProcSearchFlags = DEF);
10.  

That also covers that a single instance can return more than one. (That can be implemented later, but the interface should support it)


I thought briefly about adding a unit name, but that is maybe better left for the scope. Though the scope wouldn't return a list. And for finding methods it would be a classname or both.... But maybe that is better suited for the SCOPE?

As for overloaded, the function only searches by name. The caller has to deal with it.

[MattBradford]

Thanks — the set-of-scopes design answers everything I asked and then some, and the rename solves the overload problem better than the defaulted parameter I was worried about. Corrections from me first, then the one concern you asked for, then agreement.

You are right about Scope.FindSymbol, and I overstated it

I described the scope as handing over the same name-to-address mapping a by-name lookup would. It does not: as you say, without a unit name it starts in the locals of the current routine, then class fields, so a variable — or a global var or a type — of that name comes back before any proc. My measurement only looked like a superset because nothing in that toy program shadowed the names I probed. So the scope is not a substitute for a proc lookup, and the case for extending FindProcSymbol is better than I gave it credit for. Withdrawn.

Correction: the sysvartolstr call site is dead code

You singled out the converter as the one that would really need testing. That site — fpdebugvalueconvertors.pas ~535 — sits inside a (* ... *) block that opens at ~520, so it is commented out, and I listed it to you as a live caller. My mistake: I grepped for the call and read the surrounding lines, which does not show a comment opener fifteen lines further up. The live code in that unit is GetProcAddrFromMgr (~436), using SYSTEM_$$_GETVARIANTMANAGER$TVARIANTMANAGER, which falls in your "safe" group.

Your description — already checks dwarf first — does fit TFpDbgValueConverterJsonForDebug.GetProcAddr in fpdebugconvdebugforjson.pas. That one is live, and its FFunctionName is user-configurable, so it is the only place in the tree where an arbitrary source-level name reaches the by-name lookup.

Two smaller ones. I should not have called GetProcAddr's two lookups deliberate — it queries both, and why is not mine to assert. And the fpdbgdwarffreepascal ~2721 comment: I read it as an opinion about where a by-name lookup belongs, and it is not, it is about $fin needing to come from the same CU. Withdrawn. Also understood on breakpoints always being created — pending-until-library-load is the same reasoning as G2, and I withdraw the "fails silently" framing.

Testcase

version=030301 fixed it exactly as you said. With symbols=gw,gwset,gw3,gw4: 137 run, 0 errors, 8 failures. TTestWatches 16/16 and TTestBreakPoint 8/8 clean in all four symbol sets — every class const failure gone. The 8 remaining are TestStepOver and TestStepOver_NextOnlyFalse, 2 of 91 each, once per symbol set, failing identically every time:

Code: [Select]

90:  dcStepOver - lock cnt: Expected "3", Got "1"
91: Debugger State : Expected "dsPause", Got "dsRun"
Pre-existing on unmodified main here and perfectly reproducible, so it makes a clean before/after point and I am not chasing it as part of this. Say the word if it looks unexpected and I will raise it separately.

The one concern you asked for: changing the default to all scopes

You wrote that it is more desirable to change the default and change all callers, and asked for concerns. I have one, and it is measurable rather than hypothetical.

eval @name resolves through the scope, which for these names reaches the dwarf namespace a psfDwarfName search would consult. In an ordinary program:

Code: [Select]

eval @FPC_RAISEEXCEPTION                           -> nothing
eval @FPC_ANSISTR_DECR_REF                        -> nothing
eval @SYSTEM_$$_GETVARIANTMANAGER$TVARIANTMANAGER -> nothing
So for the current callers dwarf holds nothing and an all-scopes default changes nothing. That is the common case and it is fine.

But with user routines declared under those names — they mangle to P$COLLIDE_$$_..., so there is no linker clash:

Code: [Select]

function sysvartolstr: LongInt;   ->  eval @sysvartolstr    ^function($0000000100001870)
procedure FPC_BREAK_ERROR;        ->  eval @FPC_BREAK_ERROR ^procedure($0000000100001890)
Both resolve to the user's code, while the PE/COFF table still holds the RTL's — bare FPC_BREAK_ERROR occurs three times in that same exe. So with an all-scopes default, any caller that is not updated to pass [psfLinkerSym] would silently re-point at user code the moment somebody names a routine that way. It would present as the debugger misbehaving rather than as a name collision.

Which is an argument for the rename rather than against the default: because FindNamedProcSymbol is a new name, nothing inherits the new default by accident, and the compiler makes me visit every old call site. So I am happy with default = all scopes provided the migration is exhaustive rather than incremental — every existing by-name call converted in the same MR, with the internal breakpoints and the FPC_ helper lookups explicitly passing [psfLinkerSym]. If you would rather keep the old ones working during a transition, then the deprecated overloads should keep symbol-table-only behaviour rather than inherit the new default.

On the design, and two small notes

The set is better than the enum I proposed, and psfIgnoreCase folded in is a clear improvement over a bare True at the call site. Empty set meaning "all" reads well. Agreed on all overloads except the deprecated ones, and on AddBreak carrying it too.

One question on naming: with the future dwarf linker-names in mind, does psfLinkerSym mean the symbol tables (item 3 on your list), leaving a later flag for dwarf linker names — or did you intend it for the dwarf ones? I read it as the symbol tables, so a later addition would be something like psfDwarfLinkerName, and I will use that reading unless you say otherwise.

And a small Pascal point on the sketch: the list-returning overloads put a defaulted parameter before a non-defaulted out parameter —

Code: [Select]

procedure FindNamedProcSymbol(const AName: String; ASymInstance: TDbgInstance = nil; out ASymList: TFpSymbolArray; AProcFindFlags: TFpProcSearchFlags = DEF);
which will not compile; anything after a defaulted parameter has to be defaulted too. Easiest is to move ASymList ahead of ASymInstance, so the two defaulted arguments sit at the end. I will shape it that way unless you prefer a different order.

That is everything I need to start. Unless you would rather see it in pieces, I will do it as one MR: the flags type, the new FindNamedProcSymbol overloads on TDbgInstance and TDbgProcess, TFpDwarfInfo implementing the by-name lookup, AddBreak carrying the flags, and every in-tree caller migrated — with the testcase run before and after against the baseline above.

[Martin_fr]

You singled out the converter as the one that would really need testing. That site — fpdebugvalueconvertors.pas ~535 — sits inside a (* ... *) block

Your description — already checks dwarf first — does fit TFpDbgValueConverterJsonForDebug.GetProcAddr

That was what I referred to. It is a converter too.

TFpDbgValueConverterJsonForDebug calls "ProcSym := CurProc.FindProcSymbol(FFunctionName);"
That is a call on a user supplied name (config in IDE).  Though for all I know, it is meant to be handled by the "Scope.FindSymbol" that is performed first. (Users usually use nice names).

Testcase

I guess I need to update my 3.3.1 again.

The one concern you asked for: changing the default to all scopes

You wrote that it is more desirable to change the default and change all callers, and asked for concerns. I have one, and it is measurable rather than hypothetical.

Mine:

But, IMHO its much more desirable to change that default and change all (or change only dwarf names? but IMHO all).

I missed a "to" in that sentence. Critical. "and change it TO all".
I.e. the new default would be "all flags" (the "or only dwarf" was a 3rd alternative were the default would again be a limit, but the opposite of now).

So actually, that meant only: Change the default for the new methods.
Not: change the default of the old calls.

Which is an argument for the rename rather than against the default: because FindNamedProcSymbol is a new name, nothing inherits the new default by accident,

That is what I meant, new default on new names / old default on old names.

Old callers can then be updated one by one, and they can pass in the flags for the old behaviour, unless they are individually and deliberately wanted to be changed.

Empty set meaning "all" reads well.

All scopes. But not IgnoreCase.

That is needed, because
1) I don't thing "negative" set members are any good (e.g. psfSkipDwarf)
2) That wouldn't solve it, as then you could have "skip all"

One question on naming: with the future dwarf linker-names in mind, does psfLinkerSym mean the symbol tables (item 3 on your list), leaving a later flag for dwarf linker names — or did you intend it for the dwarf ones? I read it as the symbol tables, so a later addition would be something like psfDwarfLinkerName, and I will use that reading unless you say otherwise.

Yes, it means LinkerTables (i.e what currently is used / and only that).

It could be psfLinkTableSym instead.

And a small Pascal point on the sketch:

Yes, change order.

One MR is fine, even preferable.

[MattBradford]

All clear on the design — psfLinkTableSym, empty set meaning all scopes but not ignore-case, new default on the new names with the old names keeping theirs, ASymList moved ahead of the defaulted argument. Thanks for the "to"; that was my misreading and it makes the migration much less fraught.

Before I start: I went looking for where the dwarf lookup should go, and it is a little wider than overriding the virtual. You said one MR is fine while expecting the smaller thing, so I would rather show you now than in a diff.

The good half

TFpDwarfInfo has no scope, but it can build a location-less one and reuse everything that is already there. Measured, stopped in a proc, comparing a normal eval against one forced through a context with address 0:

Code: [Select]

eval @MarkerProc      (real RIP)      -> ^procedure($00000001000018E0)
eval @MarkerProc      (address 0 ctx)  -> ^procedure($00000001000018E0)
Identical, and the scope that gets built is TFpDwarfFreePascalSymbolScopeDwarf3, so the FreePascal FindExportedSymbolInUnit override is still in play. IsAddressInStartScope is happy with a zero address for top-level entries. So the dwarf side is genuinely small.

The half that spreads

FindExportedSymbolInUnit matches on name only — GoNamedChildEx has no kind filter, and the fsfMatchUnitName branch above it matches CU names too. So it will hand back things that are not procedures. Measured, in a program that declares "procedure Ambig" and whose own CU happens to be named ambig:

Code: [Select]

Scope.FindSymbol('Ambig')
  -> symkind=skUnit  sym=TFpSymbolDwarfUnit  symname=Ambig
P$AMBIG_$$_AMBIG is in that binary, so a procedure of that name exists and the search returned the unit instead. Filtering the result afterwards does not fix it: the scan has already stopped, so there is nothing left to filter. The kind filter has to go down into the per-CU search — which means a new member of TFindExportedSymbolsFlags, honoured in FindExportedSymbolInUnit and in the FreePascal override.

That is the part I want your view on. Is extending TFindExportedSymbolsFlags the right place, or would you rather the proc lookup did not reuse that path at all?

One thing I could not show

I expected a variable to be able to shadow a procedure of the same name across units. I built the case — proc in the program CU, var in a unit CU, neither name matching any CU name — and the procedure won. Reading FindExportedSymbolInUnits it looks order-dependent, since an external hit stops the scan and both are normally external, but I have not produced the losing arrangement, so I am not claiming it. The unit case above is the one I can stand behind.

Assuming you are happy with the flag going into TFindExportedSymbolsFlags, the MR would be: the flags type, FindNamedProcSymbol on TDbgInstance and TDbgProcess, the kind filter in the per-CU search plus the FreePascal override, TFpDwarfInfo implementing the by-name lookup, AddBreak carrying the flags, the deprecated forms forwarding with link-table-only so nothing moves under them, and every in-tree caller migrated. Testcase run before and after — my baseline on unmodified main is 137/0/8, the 8 being the two TestStepOver variants across the four symbol sets.

[Martin_fr]

but it can build a location-less one

Except the call of interest to us has

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

1. function TFpDwarfInfoSymbolScope.FindExportedSymbolInUnit(
2. ...
3.     if InfoEntry.IsAddressInStartScope(FAddress) then begin
4.  

And I don't want to do "FAddress = 0" => because riscv/avr have 0 as use-able address.

So that too, should become a flag, to ignore address.
Well, there are plenty of options here, but that is the cheapest.
- flag to procedure
- flag on field / set on create / or when address is assigned, well instead of.
- baseclass without address at all, with virtual "function CheckAddress"
...

At the moment I would go flag to procedure. Because even if one of the others is done in future, that can still be useful as alternative way. So it seems least likely to hurt in future.

which means a new member of TFindExportedSymbolsFlags, honoured in FindExportedSymbolInUnit and in the FreePascal override.

Yes. Well then with the above that is two (so far).

- fsfNoAddressCheck
- fsfOnlySubroutines

That is the part I want your view on. Is extending TFindExportedSymbolsFlags the right place, or would you rather the proc lookup did not reuse that path at all?

And probably "GoNamedChildEx"  wants to take a set instead of the series-of-bools. Keep the old, inline forward to the new. Move the code into the new that takes the enum.

IMHO re-using that path is fine.

Though that limits to global functions. (Currently they all are, except methods wich are in class nodes). But that can be worried about when there is a concrete case.

One thing I could not show

Use objdump or the dwarfviewer (components/fpdebug/test/dwarfviewer) to see what actually is in the dwarf.

If you declare a variable but don't use it (in live code) then the compiler may just drop it. Haven't checked the same for procedures, but could be too.

And then its just about unit order in the dwarf. Which is not really defined. Dwarf doesn't supply "uses" relation ships" so the search order of a scope ends at unit bounds.


There is one more thought... You don't currently need to implement it, but I want to raise it, so we don't miss an API defining detail, that may haunt us later.

Overloaded procedures.

With variables and types (at least for Pascal) the current code is (mostly / at least within one unit) fine. A variable can only exists once, and can't clash with another type. (well, there are issues, but different...)

With overloaded procedures, one name can return a list of entries. And that may be important.
Both:
- from a single unit
- from across several units (because "overload" searches other units too)

As long as we only search "top level prcos" (i.e. skipEnums) then we can just store the "ScopeIndex" where the result was found, and GoNamedChild can be taught to continue there, and another flag to that extend can be added to TFindExportedSymbolsFlags.