Is Forbid() needed for SetMethod()?

Simple question for you gurus :)

When wrapping an existing interface function, do I need to use Forbid/Permit pair:



If so, why? I mean, if SetMethod requires Forbid(), why it doesn't handle it internally then?

EDIT: updated scenario a bit.

@Capehill

I don't think it's needed, after all it only changes a pointer to another function, but I guess there is a danger of the library your changing might get flushed.
+ you might also increase the stack usage, of that function.

you might also run into issues where your patched function is executed in a forbid(), you might like to be extremely careful, not triggering any issue where a forbid() is run inside a forbid(). or a disable() inside a disable().

Since SetMethod() replace SetFunction() I would assume that Forbid() and Permit() are required like Michael Sinz do it in SegTracker.c for example.

The AROS implementation of SetFunction does Forbid/Permit internally. The original developer docs also imply that you don't have to manually Forbid/Permit:
http://amigadev.elowar.com/read/ADCD_ ... cs_3._guide/node0238.html
As for SetMethod, I'd imagine that it also does whatever locking is needed for you. Still, there's no harm in doing Forbid/Permit around the block where you patch the functions, as you won't end up being rescheduled between the SetMethod calls.

@BSzili
Yep autodocs says that forbid/permit is for pre36 lib, so logically it doesn't need forbid/permit.
I wonder why then Mike Sinz do it anyway.

NOTE
SetFunction cannot be used on non-standard libraries like pre-V36
dos.library. Here you must manually Forbid(), preserve all 6
original bytes, set the new vector, SumLibrary(), then Permit().

Forbid() and permit() are evil, do not use them unless absolutly required and keep any code between them as short as possible.

If anyone remembers XStream the proposed AHI replacement, that was eventually abandoned due to the amount of forbid()/permit() calls being used system wide which caused XStream to stutter and click. Since then a lot of work has been done to remove them from OS4.

So basically you have to use them for OS3 but can usually avoid them for OS4 unless specificly told to use them eg. getting a list on currently open screens or windows where something opening or closing a screen or window in mid scan would cause havoc.

@Kamelito

That dos.library note is just saying that dos.library <36 is "special" and is not set up normally due to it primarily being a way to call the internal functions supplied by Tripos.

I don't think that implies you do not need to Forbid()/Permit() when patching >=36 functions on any library.

Personally, I think it comes down to the library in question ... Some (like exec) would likely need a Disable if you change functions that can be called from interrupts, otherwise Forbid would be enough. You wouldn't want to change the function right as another program is calling it.

Nothing is written in the AmigaOS 4.x autodocs about forbid/permit like it was in CBM ones for SetMethod()
NOTE
THIS FUNCTION IS DANGEROUS. Don't use it. It's main purpose
is to provide ways for debuggers to hook into interfaces.

@all
I just checked the OS4 source and SetMethod contains a Forbid/Permit pair. So you are not required to set them yourself. And as Severin said:
Quote:

@LiveForIt
Quote:
The pointer change itself is not necessarily an atomic operation and there's other stuff going on too, namely changing the interface's flags and the recalculation of the lib's checksum, so all in all some locking mechanism is needed. If it had to be the fat Forbid/Permit instead of a more lightweight mutex, don't know, I'm not deep enough into this to know about all implications here.

Forbid()/Permit() would be needed if the new function acts as a wrapper for the old library function instead of replacing it entirely.



Without the Forbid() in the above example newFunc() might be called before oldFunc has been set, leading to an ISI as it tries to jump to a NULL pointer.

If the function in question is supposed to be callable from interrupts then Disable()/Enable() will need to be used instead of Forbid()/Permit().

Thanks for all comments. Example from salass00 is exactly my scenario so I think need to keep Forbid.

@salass00

That won't work anyway...

if you patch program is task1, it has all the memory and its function. task1 sets up patch, the patch is a really big complicated function that takes a long to execute maybe as much as 20 ms.

<exec switch to task 1>

task1 patches the function with new patch function.

<exec switch to task 2>

task2 start executing code from the patch function.

<exec switch to task 1>

task1 forbids exec to do scheduling.
task1 removed the patch function, and restores the old function.
task1 enable exec to do scheduling.
task1 quits, frees all memory.

<exec switch to task 3>

task3 allocates memory that privues was used by task1.

<exec switch to task 2>

task2 continue executing code from program counter it last executed code.

And now you got typical ISI or DSI error, we all love.

So forbid / permit does really help anything.

@LiveForIt

The discussion was about installing a patch and not removal of one (as was my example code).

You are right though that removing an installed patch causes some problems in that after calling IExec->SetMethod() to restore the old function it is impossible to be 100% sure that there is not a program still using the patch function or just about to call it.

About the best you can do is add a large delay after and hope that it's enough.