How does my static variable survive expunge?

[hceline]

I got a library that does SetFunction() on AddAppMenuItem(), AddAppIcon(), and AddAppWindow().
They in-turn call another function in the same .c file that adds the Menus/Icons/Windows to a tracking-list.
In this process it increments its own lib_OpenCnt with one if it finds the tracking-list empty to begin with.
A cleanup function elsewhere removes items from the tracking-list and decrements the lib_OpenCnt when the list gets empty.

But sometimes the tracking-list would be emptied without the involvement of said cleanup function, which lead to a runaway lib_OpenCnt.
So I had to limit this to only increment lib_OpenCnt in the first call to this function.
I made a function-local static BOOL to identify the first call to this function.

Today it came back and bit me.....
Apparently this variable survives the library being expunged (including the original workbench.library functions being successfully SetFunctioned back) and opened again.

I fixed it by placing the variable in a struct that gets allocated in __UserLibInit(), and also set its initial value there.

But I am still left with the question, how did the variable survive?
I've been reading up on the little I could find on SetFunction(), and searching trough the library code trying to figure out this, but I'm at a loss to what I'm even looking for.
Is this all SetFunctions doing?
In that case, how does it know of this function at all?
If not SetFunction, what can do this, there are no tasks or residents accounting for keeping it alive trough a expunge.

The code in question is here: https://github.com/Hagbard-Celin/gal...e/Library/wb.c
Since the function in question is riddled with #ifdef/KPrintF() and thus quite hard to read, I'm pasting in a cleaned up version here:

AppEntry *new_app_entry(
    ULONG type,
    ULONG id,
    ULONG userdata,
    APTR object,
    char *text,
    struct MsgPort *port,
    WB_Data *wb_data)
{
    AppEntry *entry;

    // This is the immortal variable in question
    static BOOL dejaVu=FALSE;

    // Allocate new entry
    if (!(entry=AllocVec(sizeof(AppEntry),MEMF_CLEAR)))
        return 0;

    // Fill out AppEntry
    entry->type=type;
    entry->id=id;
    entry->userdata=userdata;
    entry->text=text;
    entry->port=port;
    NewList((struct List *)&entry->menu);

    // AppIcon?
    if (type==APP_ICON)
    {
        struct DiskObject *icon_copy;

        // Copy icon
        if (icon_copy=L_CopyDiskObject((struct DiskObject *)object,0,wb_data->galileofm_base))
        {
            // Use copy
            entry->object=icon_copy;
            entry->flags|=APPENTF_ICON_COPY;
        }

        // Couldn't copy
        else
        {
            FreeVec(entry);
            return 0;
        }
    }

    // Otherwise, save object pointer
    else entry->object=object;

    // Lock patch list
    L_GetSemaphore(&wb_data->patch_lock,SEMF_EXCLUSIVE,0);

    // Is this the first entry?
    if ((IsListEmpty((struct List *)&wb_data->app_list))&&(!(dejaVu)))
    {
        dejaVu=TRUE;

        // Bump library open count so we won't get expunged
        ++wb_data->galileofm_base->ml_Lib.lib_OpenCnt;
    }

    // Add to list
    AddTail((struct List *)&wb_data->app_list,(struct Node *)entry);

    // Unlock list
    L_FreeSemaphore(&wb_data->patch_lock);
    return entry;
}

[pipper]

I think DOpus is using its own libinit code, which is responsible for initializing static variables. If the library was closed and reopened in the same place and on top the static initializers are not executed, you’ll end up seeing the same values.
If you’re using the GCC/libnix combo, the DOpus libinit code must call the relevant initialization functions in libnix.

[hceline]

I use SAS/C.
You are right, it uses its own _LibInit(), _LibOpen(), _LibClose() and _LibExpunge(), and it almost certainly was loaded again at the same location.
But, with the caveat that I do not yet fully grasp the library init code, I can not see that those are substantially different form the originals in sc:source/libinit.c.

Edit: After doing a diff, I can conclude that he only difference is removing #ifdef/#ifndef for DEVICE and ONE_GLOBAL_SECTION. Diff file attached.

Edit 2: Not quite only, it removes calls to __libfpinit()/__libfpterm() too.

[Thomas Richter]

Originally Posted by hceline

I made a function-local static BOOL to identify the first call to this function.

Today it came back and bit me.....
Apparently this variable survives the library being expunged (including the original workbench.library functions being successfully SetFunctioned back) and opened again.

Remember that Expunge() does not necessarily remove the library from memory. It may fail. Only if this function returns non-zero, it has done so.

Originally Posted by hceline

Is this all SetFunctions doing?

The original SetFunction only patches a function entry, and that is it. However, depending on what is installed in your system, it may do a bit more. SaferPatches will (in some incarnations) create a trampoline function for the patched function such that patches can be removed in an order that is different from the inverse installation order, and it will also patch the expunge vector to learn when it has to release the trampoline functions. But that works according to the specs, i.e. it only releases trampolines if the library has been really removed.


This has some implications on how SetFunction() has to be called, thus for example, to remove a patch, you shall call SetFunction again with the vector returned from the first call (which is a plausible way of assuming how things work), and not with a vector the the patching program obtained by peeking the library base itself before attempting the patch.


Also, there are plenty of pitfalls in using semaphores in LibOpen and LibClose. Shortest version of the story: Don't. You'll break things easily, unless you really really know what you are doing. Don't Wait() in LibOpen and LibClose(), don't break the Forbid(). If there is anything asynchronously to be done, do it in LibInit() as part of the ramlib process, there nothing bad can happen.


Even more so, don't break Forbid() in Expunge(), avoid, avoid. Again, unless you totally know what you are doing. I'm saying, because the Os 3.9 boopsi classes were full of naive library implementations that had races.

[hceline]

Originally Posted by Thomas Richter

Remember that Expunge() does not necessarily remove the library from memory. It may fail. Only if this function returns non-zero, it has done so.

I must admit that I never checked the return value of Expunge(). But I did check that the library was no longer on the system library list in ShowConfig/ARTM. That is the same, is it not?

Originally Posted by Thomas Richter

The original SetFunction only patches a function entry, and that is it. However, depending on what is installed in your system, it may do a bit more.

My testing setups runs only a clean OS-install with SYS:WBStartup renamed to disable it.
The only thing I've added is SegTracker, PatchWork, MuForce and MuGuardianAngel that runs in some combination, or not, depending on what mouse-buttons I hold down during boot.
Disabling them makes no difference in this case.
Anyway, this confirms that the issue is not related to SetFunction() itself.

Edit: I also got latest version of P96 in the test setups. I slipped my mind, when typing the above. Probably because it could not possibly be related.
Also for completeness I might add that I've added disassembler.library to the standard mmu.library install. (I believe that is not done by the standard installer.)

I am beginning to wonder if the SAS/C Global Optimizer might in some way be the culprit.
Going off to try compiling the library with NoOptimize.

Originally Posted by Thomas Richter

Even more so, don't break Forbid() in Expunge(), avoid, avoid. Again, unless you totally know what you are doing. I'm saying, because the Os 3.9 boopsi classes were full of naive library implementations that had races.

Not related but since you mention it, I've been wondering. Does KPrintF() from debug.lib break Forbid()?
I've been thinking, it is a IO operation and all IO operations can break Forbid(), so yes?
But, I never seen any warning about KPrintF() and Forbid() anywhere. If it did break it, there would have been a warning somewhere, as it is a function one might likely put anywhere in the code. And the autodoc only says for KPutChar() that it blocks until completion, so no?

[hceline]

Originally Posted by hceline

I am beginning to wonder if the SAS/C Global Optimizer might in some way be the culprit.
Going off to try compiling the library with NoOptimize.

Well that did not make any difference.

I also checked the free memory before first launch of the main exe after doing a FLUSH with MemLeakZ, against the state after quitting and doing another FLUSH.
There is a loss, but it fluctuates, and most of the time it's too small for it to contain the library code.
The lowest loss I got was 2264 bytes lost. With a library "Maximum code size" as given by slink at 218036 bytes.
There where 26 libraries open before the test according to ARTM, an 26 open after.
Still when starting the exe again my immortal variable retained the TRUE value from first run. And was located at exactly the same address as in first run.

[meynaf]

I suggest you try putting another value than FALSE in your immortal variable's initializer and check what's there the very first time your app entry is called.

[hceline]

Originally Posted by meynaf

I suggest you try putting another value than FALSE in your immortal variable's initializer and check what's there the very first time your app entry is called.

And that finally gave some clue as to what is happening.
I changed the "FALSE" to "42" and upon first call to new_app_entry() it is "0".
All subsequent calls give "1" as before.

I assume that this means pipper was right about static initializers not being executed.
If that is the case, how does one fix it?

[meynaf]

I would fix that by not using static initializers at all.
As you're in a library, you should have some kind of library base somewhere. In the library struct, the neg area is for function calls, the pos area is for library data : i'd put the BOOL there.

[hceline]

Originally Posted by meynaf

I would fix that by not using static initializers at all.
As you're in a library, you should have some kind of library base somewhere. In the library struct, the neg area is for function calls, the pos area is for library data : i'd put the BOOL there.

I did approximately that to fix this particular issue to begin with. This is only me trying to understand why it failed as it was. (And nevermind that I discovered that I could not have used a local static anyway, because I had to reset it in the before-mentioned cleanup function for it to work for subsequent program restarts if not flushing the library in-between.)

Originally Posted by hceline

I fixed it by placing the variable in a struct that gets allocated in __UserLibInit(), and also set its initial value there.

I also searched the library code and found exactly one other occurrence of a function-local static variable.
In boopsi.c:L_AddScrollBars() starting at line 110.

It begins like this:

struct Gadget *__asm __saveds L_AddScrollBars(
    register __a0 struct Window *window,
    register __a1 struct List *list,
    register __a2 struct DrawInfo *draw_info,
    register __d0 short flags)
{
    struct Image *image[4]={0,0,0,0};
    struct Gadget *gadget=0;
    short a;
    static struct TagItem
        map_tags[]={
            {PGA_Top,ICSPECIAL_CODE},
            {TAG_END}};

So I added:

    KPrintF("!!!boopsi.c line: %ld BEGIN map_tags[0]: %lx at %lx\n", __LINE__, map_tags[0].ti_Tag, &map_tags[0].ti_Tag);
    KPrintF("!!!boopsi.c line: %ld BEGIN map_tags[1]: %lx at %lx\n", __LINE__, map_tags[0].ti_Data, &map_tags[0].ti_Data);
    KPrintF("!!!boopsi.c line: %ld BEGIN map_tags[2]: %lx at %lx\n", __LINE__, map_tags[1].ti_Tag, &map_tags[1].ti_Tag);

And the output of those lines on first run was: (Edit:Please ignore the errant naming in the KPrintF() output)

!!!boopsi.c line: 124 BEGIN map_tags[0]: 80031009 at 8FB73B0
!!!boopsi.c line: 125 BEGIN map_tags[1]: 80040003 at 8FB73B4
!!!boopsi.c line: 126 BEGIN map_tags[2]: 0 at 8FB73B8

So that apparently got the right values.

And after quitting, doing FLUSH and restarting:

!!!boopsi.c line: 124 BEGIN map_tags[0]: 80031009 at 8FB6F60
!!!boopsi.c line: 125 BEGIN map_tags[1]: 80040003 at 8FB6F64
!!!boopsi.c line: 126 BEGIN map_tags[2]: 0 at 8FB6F68

And it does not show up at same address when reopening library after flush.

The only* difference I could see between these functions being the one that worked having "__asm __saveds", I tried adding __saveds to new_app_entry().
No dice. Testing with "42" still in the initializer, it still starts out at "0", and then "1" on every subsequent call regardless.


I'm even more confused now.
Why would the static initializers work in L_AddScrollBars(), and not in new_app_entry()?


*Except that L_AddScrollBars() does not change map_tags after initialization.

[pipper]

Try "avail flush" in the shell to truly get rid of libraries after they have been closed to 0 users.

I suggest disassembling the code and find out where the variable is stored.
The compiler may decide to place it into the code segment and actually have the code segment contain the initial value. But this way the variable could only be restored upon reloading the entire code segment. It would not be enough to close the library, expunge it and reopening it. The compiler may decide to put it into a data segment. But then again, closing the lib and reopening would not reset the value. Zero-initialized global variables can also be stored in a bss segment. Libnix (IDK if that is what SAS/C is using as libc?) is at least attempting to restore the bss segment to all-zero:
https://github.com/bebbo/libnix/blob...libinit.c#L175
But you only get this behavior if youre actually using Libnix' startup code... which DOpus does not.

I think, static variables have undefined semantics in the context of Amiga libraries and thus should be avoided.
Are we expecting one copy of each static variable per library user ( I think Libnix has provisions to do that called "Multibase Library" and the startup code is in https://github.com/bebbo/libnix/blob...tup/libinitr.c )? Or is a static considered global across all processes using the library?

[meynaf]

It is possible that the compiler found out that map_tags is never modified and treated it as constant data - which needs no init code at all.
In that case, it works by accident...
Normally, this should be declared as static const.

[hceline]

Originally Posted by pipper

Try "avail flush" in the shell to truly get rid of libraries after they have been closed to 0 users.

I have been doing the equivalent of "avail FLUSH" on each test. Only using MenLeakZ from aminet to avoid having a shell open that consumes a little memory for each time I run "avail Flush".
I've verified with ARTM/SysInfo that the library is really gone from the system library-list. And I assume this means all the memory belonging to the library should have been freed.

Originally Posted by pipper

The compiler may decide to place it into the code segment and actually have the code segment contain the initial value. But this way the variable could only be restored upon reloading the entire code segment. It would not be enough to close the library, expunge it and reopening it. The compiler may decide to put it into a data segment. But then again, closing the lib and reopening would not reset the value.

The smallest memory loss I've recorded if I do:
1 a clean boot
2 then a FLUSH
3 run the main exe and quit it
4 and then another FLUSH
and checking that the system library list after 2 was equal to the library list after 4,
.. is 2264 bytes.

And given that the "Maximum code size" as given by slink for the library was at the time 218036 bytes, I assume the library code is freed, and must be reloaded on next OpenLibrary().

Originally Posted by pipper

Zero-initialized global variables can also be stored in a bss segment. Libnix (IDK if that is what SAS/C is using as libc?) is at least attempting to restore the bss segment to all-zero:
https://github.com/bebbo/libnix/blob...libinit.c#L175
But you only get this behavior if youre actually using Libnix' startup code... which DOpus does not.

Initializing it to non-zero value on declaration does not appear to change anything.

Originally Posted by pipper

I suggest disassembling the code and find out where the variable is stored.

That would apparently be the next logical step. But I'm verging on abandoning this quest.

Especially if this holds true:

Originally Posted by pipper

I think, static variables have undefined semantics in the context of Amiga libraries and thus should be avoided.

And I think this closes the case:

Originally Posted by meynaf

It is possible that the compiler found out that map_tags is never modified and treated it as constant data - which needs no init code at all.
In that case, it works by accident...
Normally, this should be declared as static const.

I guess I've learnt not to use static variables in library code after this.


Thank you all so much for bearing with me.

[Thomas Richter]

Concerning static initializers: How did you compile your source? Because depending on the options, SAS/C will make all non-automatic objects part of the library base, and such objects whose lifetime starts at the point the library is initialized, and whose life time ends when the library is expunged. Please study the SAS/C manual concerning the LIBCODE command line object.

[Thomas Richter]

Originally Posted by pipper

I suggest disassembling the code and find out where the variable is stored.
The compiler may decide to place it into the code segment and actually have the code segment contain the initial value.

Actually, that would be the "text segment", and SAS/C only places there "const" objects, but not objects of static or external linkage. Those are either placed in the data segment (for HUGE data model) or in the __MERGED segment (in the SMALLDATA model). With LIBCODE, the rules are again different, but this is all in the SAS/C manual

Originally Posted by pipper

Zero-initialized global variables can also be stored in a bss segment.

For a library, it is rather bad practise to store objects of static or external linkage in separate segments because that decouples them from the lifetime of the library. Depending on compiler options, either you get one instance of the library base carrying such objects which is then shared amongst all users opening the library, or you get one library instance per attempt to open the library, but then such objects are created on LibOpen and cease to exist on LibClose().



What exactly is right depends on your application.

Originally Posted by pipper

I think, static variables have undefined semantics in the context of Amiga libraries and thus should be avoided.

Exactly that, see above.

Originally Posted by pipper

Are we expecting one copy of each static variable per library user ( I think Libnix has provisions to do that called "Multibase Library" and the startup code is in https://github.com/bebbo/libnix/blob...tup/libinitr.c )? Or is a static considered global across all processes using the library?

That is a matter of compiler options. As said, it's in the SAS/C manual.



Even the way how the Amiga system libraries are compiled is not consistent. Some use LIBCODE, others do not, and some use some really dirty tricks to access data relative to the library base even without using LIBBASE.


I would need to look up, but I think there is example library code somehwere in SAS/C which may help.