Avoid name space collision for the global variables PC, SP and BC whe…

[pkooiman]

…n readline is dynamically loaded.
This fixes #458 for all affected simulators.

[psco]

Actually I think the approach with renaming the offending names via a macro (similar to the #define PC PC_Global approach) is cleaner. It directly addresses the problem, namely that there is a name clash. Furthermore the static approach appears to preclude future uses in other modules of the variables made static.

[pkooiman]

Since Mark said the exact opposite yesterday, I will wait for his comments before making any changes.

[psco]

My point is that the static approach works today but we do not know whether a future change requires access to one of these variables. If this should happen, the renaming via a macro is needed anyway. I do not see a downside to the macro approach.

[markpizz]

My point is that the static approach works today but we do not know whether a future change requires access to one of these variables. If this should happen, the renaming via a macro is needed anyway. I do not see a downside to the macro approach.

That is true, but in each of the "static:" cases, there is one source module which completely implements the processor so any theoretical future addition of devices would not be messing with the processor's internal registers. However, there really isn't a downside to the macro approach. The simname_defs.h is where definitions and globals shared between different source modules belongs.

FYI, you can "git push -f" to change an existing pull request if changes are needed after review.

[rcornwell]

Only problem I can see with global renaming of something like PC is it might confuse people trying to debug a simulator as it would be called PC_Global instead of PC like in the source.

[markpizz]

Only problem I can see with global renaming of something like PC is it might confuse people trying to debug a simulator as it would be called PC_Global instead of PC like in the source.

True, but it depends on the level of debugging they are doing. The name PC_Global is what the OS debugger sees, but the simh REG variable is still whatever the simulator author named it and is visible from the sim> prompt by that name.

These changes are only for much older simulators which OS level debugging is quite unlikely. New simulators being developed now will naturally have locally scoped variable names (static) rather than the default of global the author(s) of the older simulators had.

The original changes that addressed the PC_Global case (c2cef3c) have been in the codebase for more than 10 years.

[pkooiman]

If someone in the future needs to do major work on one of the affected simulators and is annoyed enough at having to use "PC_Global" instead of PC in the debugger, I suppose they have the option of manually renaming PC to something else. This seems to have happened with the hp2100 simulator according to hp2100_bugfixes.txt.

I am happy to amend the pull request to use the #define everywhere, let me know if that is the preferred option.

[psco]

I would prefer the renaming option via #define.

[markpizz]

I would prefer the renaming option via #define.

Agreed.

[pkooiman]

Ok, changed to #define everywhere.

[psco]

Thank you - this looks good. I checked and the issue was present even in Ubuntu 24.10 (in my previous attempt I had not installed libedit-dev and without editline functionality there is no crash).

[pkoning2]

Libraries that use names like this clearly have a design issue; it's annoying to have to work around errors like that.

[rcornwell]

I do not believe that PDP10 simulators include ncurses. I have deposited values into PC without issue. Perhaps only those simulators the use ncurses be fixed.

[markpizz]

No simulator explicitly interacts with ncurses, or readline. Those are happening within the SCP functionality.

The PDP10 simulators code that was changed specifically referred to the PC global variable which is indeed shared between the simulator AND whatever shared library that also exports PC, SP and BC. The failure case was very observable at the sim> prompt when the simulator had a SP variable which was manually changed by the simulator or by a SCP command and you could see the failure. This problem DOESN'T happen on all platform situations, but it does happen on some platforms. Sharing the PC CAN NOT easily demonstrate the failing problem like the shared SP could in a segfault, but I hardly believe that you would want some external library to change the value of the simulator's PC variable whenever it feels like it.

[markpizz]

You could easily correctly argue that the problem is within ncurses or readline or whatever logic has the shared global variables, BUT getting that changed everywhere would be a much bigger hill to climb than the proposed fix to the simh simulators.

[rcornwell]

Why is ncurses exporting these variable? I checked the ncurses manual and it does not list these variables.

[pkooiman]

That’s due to an unfortunate design choice made by ncurses. See here for the definition of the global exported SP symbol, PC and BC are exported similarly. Imagine how much “fun” it is chasing down the source of crashes due to clashes with those exported symbols, which as you rightly say are not documented.

[markpizz]

Why is ncurses exporting these variable? I checked the ncurses manual and it does not list these variables.

I have no idea. The original issue / commit (c2cef3c) specifically adding the "#define PC PC_Global" was added to some simulators a bit more than 10 years ago. I don't recall the particular detail that caused that one to be tracked down. There were some 10 simulators changed at that time. I don't remember when your simulators were added to the simh/simh repo, but probably after that.

[pkoning2]

I'm wondering if it might not be simpler/cleaner to do something like:

#define SP _dont_use_SP
#include <ncurses.h> /* or whatever it's called */
#undef SP

where the offending ncurses include file is referenced. That way the simulators will just use the natural names and the offending globals from ncurses become inaccessible.

[pkooiman]

That won't work unfortunately. The clash happens at runtime when the dynamic linker loads the ncurses libraries. It sees that both the simulator and the library export a symbol called "SP" and figures it must be the same symbol. In other words, because the ncurses dynamic libraries export a symbol called "SP" there is no way any program using the libraries can have a global (non-static) symbol also called "SP" without a clash occurring.

[pkoning2]

Which OS? I thought that the dynamic linking rule says that the main program can override a global from the library, and what's in the main program then takes effect.
But I suppose avoiding the wrongheaded symbol is a simpler answer. I'm just a little bothered by it being a per-simulator workaround. Could the #define go into sim_defs.h so it applies to everyone?

[pkooiman]

The main program overriding the global from the library is the problem, because the library is trying to write to a symbol SP that should be internal to the library but it is in fact overwriting the main program's SP variable.
The per-simulator workaround is definitely not ideal but I don't know how you'd make sure that a wholesale #define in sim_defs.h does not have unforeseen effects on any of the simulators.

[markpizz]

It can't go in sim_defs.h since other simulators already have a #define SP which will then clash.