#61 Use less global and static variables

UrJTAG 0.x
open
Kolja Waschk
UrJTAG (99)
5
2008-03-30
2008-03-30
Kolja Waschk
No

Currently, the code is used only by the jtag shell for one chain at a time, but it might be used for more than one in the future (especially when it is used as a library).

Some drivers and subsystems use global variables for state information. These certainly would cease to work in a multi-threaded environment.

Discussion

  • Kolja Waschk
    Kolja Waschk
    2008-03-30

    Logged In: YES
    user_id=478715
    Originator: YES

    A quick overview about currently defined global variables can be printed with

    objdump -j .bss -t [executable]

     
  • Arnim Läuger
    Arnim Läuger
    2008-04-01

    Logged In: YES
    user_id=156321
    Originator: NO

    Thanks for posting the objdump command. That's a very good starting point for investigations.
    Now I see that there are a bunch of globals in the svf and bsdl modules. Some of them are resolved easily, some require more effort. But to start at all, we'd require a good hook for the globals.

    First idea: use chain_t as the "mother of all variables" since all jtag actions are performed on the current chain. I.e. add new members like "bsdl_t" to chain_t that contain global/static information for a submodule. It would act then as a descriptor for each library instance.

    Or we define a new toplevel structure where things are hooked to.

     
  • Kolja Waschk
    Kolja Waschk
    2008-04-02

    Logged In: YES
    user_id=478715
    Originator: YES

    It has to be differentiated between variables that really are global (such as configuration settings, data directory etc) and others that actually are relevant only locally, but have to be retained between calls to the functions using them (such as cable state). The latter certainly should go to chain_t or descendants of it, as "low" as possible. The "real globals" should be put into another structure with a pointer to it in chain_t; probably a good candidate to become "urjtag_t"...

    That'd be the first step towards implementation of a config mechanism etc.

    http://sourceforge.net/tracker/index.php?func=detail&aid=1892199&group_id=193266&atid=944728

    typedef struct urjtag_t struct urjtag;
    struct urjtag
    {
    chain_t **chain; // might also be implemented as a "chained list of chains"
    int chains; // number of chains
    // TBD: configuration items like data directory
    // TBD: URFP variables & functions
    // TBD: URFP debug channel configuration etc.
    }

    However, I suppose it's also okay to put the globals into chain_t for now, and wait for the generic configuration mechanism to evolve during other development e.g. of URFP server. Once they're identified and "de-globalized", it's probably no big deal to move them up one level from chain_t to urjtag_t when appropriate.

     
  • Arnim Läuger
    Arnim Läuger
    2008-04-02

    Logged In: YES
    user_id=156321
    Originator: NO

    SVN trunk r1149 has seen reductions of global variable usage for BSDL and SVF submodules. BSDL seems clean now, but SVF has one global remaining: max_time_reached. It's used for timing RUNTEST commands with an OS signal and it would be hard to remove the global here as well.
    However, by review I'd say that the code is broken nonetheless. I'll have to dig through patch '[ 1194140 ] SVF better RUNTEST timing' which introduced this signalling scheme. Maybe I can get rid of this last global as well.
    BTW, the two BSDL globals were added to chain_t inside a dedicated structure. SVF globals could be handled within the files inside src/svf.