Avoid accessing instance variables in inline functions when compiling with msvc

[qmfrederik]

DPSOperators.h exports various inline functions which access the methods variable of the NSGraphicsContext class. This will result in accessing instance variables across module boundaries if these functions are used in external libraries.

Accessing instance variables across module boundaries is not supported when building on Windows using the Visual Studio toolchain.

This patch addes a methods function on NSGraphicsContext which returns the variable, and reworks the inline functions to use the methods function instead of directly accessing the variable.

This patch is based on @gcasa's work.

[qmfrederik]

@gcasa @fredkiefer Would you mind approving the workflow for this PR, so the CI pipeline can kick in?

[gcasa]

These changes look ok to me, but I recommend waiting for @fredkiefer to approve as well since he is the gui maintainer.

[fredkiefer]

This looks fundamentally broken to me. The whole idea behind the implementation of these inline functions was to avoid a method call and speed up things that way. If we already do a method call to get the method table why not instead call the specific method directly?
Could you please do a few performances measurements for these different approaches and report back? Of course on a Linux/BSD system and for both the new runtime with clang and the old one with gcc. Only then will we be able to decide on how to get rid of the current optimisation in an orderly way.

[qmfrederik]

@fredkiefer Thank you for your feedback.

If we already do a method call to get the method table why not instead call the specific method directly?

1. Currently, there's no method which returns the methods variable. I've split off the change that adds the methods method in NSGraphicsContext: Add methods method #179 . I hope that change is non-contentious.
2. This change is specific to the Visual Studio compiler, which does not allow you to access instance variables across module boundaries. These functions are used in libs-back, where they are inlined, and this results in a compiler error.

One alternative may be to rewrite the patch like this:

{
  NSGraphicsContext *ctxt = GSCurrentContext();
  if (ctxt != nil) {
#ifdef _MSC_VER
    ([ctxt methods]->NSBeep)
#else
    (ctxt->methods->NSBeep)
#endif
      (ctxt, @selector(NSBeep));
  }
}

Thoughts?

[fredkiefer]

I have the impression you did not understand my argument. Why not write your example as

{
  NSGraphicsContext *ctxt = GSCurrentContext();
  if (ctxt != nil) {
    [ctxt NSBeep];
  }
}

[tedge]

Accessing public ivars is an ObjC language feature. If the VS toolchain doesn't support this, then it's a broken implementation; The bug should be fixed there, not in GNUstep.

There was a similar issue accessing external ivars on libobj2 awhile back, and it was fixed in the compiler, not in GS:
https://lists.gnu.org/archive/html/discuss-gnustep/2017-12/msg00130.html

[qmfrederik]

Thanks @fredkiefer and @tedge . Yes, this is a bug in the toolchain, but it's not uncommon for projects to try to work around bugs in toolchains either, right?

What about something like this:

static inline void
NSBeep(void)
{
  NSGraphicsContext *ctxt = GSCurrentContext();
  if (ctxt != nil) {
#ifdef _MSC_VER
    [ctxt NSBeep]
#else
    (ctxt->methods->NSBeep)
      (ctxt, @selector(NSBeep));
#endif
  }
}

This would isolate the change to code compiled with the Visual Studio compiler only, and not impact the other code paths.

Would you be willing to consider such a patch?

[fredkiefer]

This approach looks a lot better to me.

[qmfrederik]

@fredkiefer I've reworked the patch, which now consists of two commits:

1. The first commit refactors DPSOperators to use macros for all the inline functions. The reasoning is that all these functions are very similar, and using a macro would help avoid any inconsistencies. I can undo this change if you so prefer.
2. The second commit adds the #ifdef _MSC_VER statements to use the optimized code path on all compilers except for msvc, and uses the slow path (but avoids the use of instance variables) on msvc.

Let me know what you think.

[qmfrederik]

Looks like I'll need maintainer approval to restart the CI job; CI did run in my fork and came back green: https://github.com/qmfrederik/libs-gui/actions/runs/4610037108/

[qmfrederik]

@fredkiefer Thank you for your review. I hope I've addressed your feedback. The CI in my fork comes back green, but looks like I'd need your approval for the workflow to run in the context of this PR. Let me know if you have further feedback on this, greatly appreciate your assistance.

[fredkiefer]

How should we proceed here? I don't have the impression that this changes has been thoroughly tested for both cases. If you could fix the one remaining issue and test for the non MSC case as well, I am willing to merge.

[gcasa]

Hey guys. I think we should go ahead and merge this. @fredkiefer @qmfrederik @triplef

[fredkiefer]

Hi Greg,

if you think you should merge a broken change, feel free to do so. I am out of this now and leave it up to you to further ignore my comments.

[gcasa]

@fredkiefer is there a particular reason why you feel you have to have this kind of attitude? You yourself had said you thought things looked better. The fact of the matter is that direct access does NOT WORK on msvc's version of clang and our code is abusing this in a questionable manner. If you have a better solution then PLEASE BY ALL MEANS propose it.

[gcasa]

I didn't ignore your comments. I was reacting to them unless I read things wrong. But I must say that I DEEPLY LOATHE when you start acting like a jerk over simple comments or misunderstandings.

And yes I am calling out this behavior publicly because you have done this just one too many times for my taste.

I don't understand the source of your incivility.

Given your comment about "how should we proceed" I was simply voicing my opinion. Sorry if you think that was "ignoring your comments".

[gcasa]

I have tested it on both Linux and windows. @fredkiefer please be more civil in the future. Under no circumstances is it ok to take an attitude with other developers as you did earlier. Not from you or any other developer in this project. Our unwritten COC is "don't be a jerk."

So I leave this up to you since you are the gui maintainer. Feel free to ignore MY opinion if you like.

[triplef]

Hi Greg, just to chime in here I believe Fred was referring to the remaining open issue marked above. He already said he’d be fine to merge once that is fixed.

[gcasa]

Right. I get precisely what @fredkiefer was trying to say.

I was simply chiming in and giving my opinion since I have tested the code in both places and because it has been languishing since June in this PR.

And, if I was wrong, which I frequently am, he should have corrected me in a civil discussion. But saying "I'm done, your problem, I'm waking away" and acting like he did is utterly inexcusable and uncalled for.

I absolutely won't, under any circumstances, allow or accept that kind of childish behavior on this project plain and simple. If someone wants to act like that they can do it elsewhere, not here.

Be civil. It's really easy to do.

[gcasa]

@fredkiefer I will be more careful replying in the future. I jumped to the conclusion that the code was ready, and for that I was wrong. But you must be more civil when there are disagreements going forward.

[qmfrederik]

Thanks @fredkiefer for spotting that. I believe I've addressed your comment and CI looks green. Let me know if there's anything else you'd need from me here.

[qmfrederik]

@fredkiefer This was merged via 7de3485, but that commit does not appear to be in the master branch. Do you know what happened here? Was that intentional, or should I open a new PR to get these changes merged (again)?

[fredkiefer]

I have no idea what happened here. From the history here it looks like the branch was properly merged but there are no traces of it in the master branch. There are only two possible explanations, either GitHub never merged it or somebody used a force commit to completely remove it again. I did receive a mail for the merge and the next day there was a mail about a change by Gregory to the master branch without any details.

It seems best to open another pull request with the same content and we try to merge that again and this time we check carefully what happens.