Anders Melander

The test is IMO doing too much so it's hard to tell where the problem originates. Since we've been focusing on the TEventItemHolder stack I would start by verifying that the original code fails (although it's obvious that is has the ABA problem) and the new code doesn't fail. Then I would validate TMonitor.Wait and only then would I validate TThreadedQueue.PopItem.

Not in this single case, I agree, but beware of tunnel vision. Regardless the obstacle is probably more a question of resources and priorities. It might also not be easy to shoehorn something like this into the existing compiler. If it's implemented like a traditional one-pass compiler it will be using various optimizations that can only be made because it's a one-pass compiler.

It isn't. The type and offset of the members needs to be known too: type TMyRecord = record Foo: integer; Bar: string; end; TMyClass = class private FFooBar: TMyRecord; public property Foo: integer read FFooBar.Foo; property Bar: string read FFooBar.Bar; end;

You're missing an "n" in Exchange and the NewValue and CurrentValue parameters should be declared as const or [ref].

It's been raining so here you go (completely untested): type TEventStack = record Counter: int64; Head: PEventItemHolder; procedure Push(EventItem: PEventItemHolder); function Pop: PEventItemHolder; end; var EventCache: TEventStack; ... EventItemHolders: TEventStack; procedure TEventStack.Push(EventItem: PEventItemHolder); var Current, Next: TEventStack; begin repeat // We don't need to copy atomically since the test below will detect tearing // but since the members should be aligned tearing should not occur anyway. Current := Self; EventItem.Next := Current.Head; Next.Head := EventItem; Next.Counter := Current.Counter + 1; // I'm assuming TInterlocked.CompareExchange returns a boolean: True on success. until TInterlocked.CompareExchange(Self, Next, Current); end; function TEventStack.Pop: PEventItemHolder; var Current, Next: TEventStack; begin repeat Current := Self; if (Current.Head = nil) then Exit(nil); Next.Head := Current.Head.Next; Next.Counter := Current.Counter + 1; until TInterlocked.CompareExchange(Self, Next, Current); Result := Current.Head; end; I've made the two functions members of the record to get rid of the first parameter. What you (or someone else) need to do is provide an implementation of TInterlocked.CompareExchange, AtomicCmpExchange or CAS that handles 16 bytes and returns a boolean indicating success.

Oh. I generally don't bother with attached or linked files. However from a brief glance at your code this looks fishy: until (AtomicCmpExchange(Stack.Spin, Spin + 1, Spin) = Spin) and CAS(Result, Spin + 1, Result.Next, Spin + 2, Stack); I would think you'd need to test and update both Spin and Next in one go. That's why you need the 128 bit CAS. I don't see the need for both the AtomicCompareExchange and the CAS. You also need to modify the Push functions since it has the same problems as the Pop. I might give it a go this week-end (if the weather is bad 🙂) but I suspect someone else might beat me to it.

Ouch! One would have thought they knew not to write code like that. As far as I can tell it can be solved "simply" by changing the head of the two stacks from a pointer to record containing the pointer and a counter: type TEventStack = record Counter: int64; Head: PEventItemHolder; end; var EventCache: TEventStack; ... EventItemHolders: TEventStack; ...and then change the Push and Pop functions to increment the transaction counter on each operation. Finally we just need a version of AtomicCmpExchange that operates on 128 bits (e.g. the 16-byte CAS mentioned above) to update the transaction Counter and the Head pointer in one go. I believe this will take care of the ABA problem.

Classes are reference types.

You mean a preprocessor. Adding a preprocessor would amount to the same as making the compiler multi-pass.

Yes, probably although I don't think the front end is enough. But more importantly I think it's about cost/benefit and the limited resources available to them.

You are not stuck. There's always choice. I'm assuming you've chosen to stick with Delphi because you like the language. With the benefits Delphi provides comes some limitations. That's just part of the equation. Because it's a one-pass compiler (mostly). The benefit is that it is fast. The price is that there are some things that are not possible. Reference types can be forward declared because their size are always known (=SizeOf(pointer)). The size of record types are not known until they have been fully declared. This means that the compiler cannot determine the layout of a record type if it contains other record types that have not been fully declared, and it needs to know the layout in order to generate the code that uses it. This problem can be solved, with certain limitations, while still staying a one-pass compiler, but at a cost of added complexity in the compiler and longer compile time.

Without access to the compiler source it's hard to tell, but I'll bet there are some. For one, as far as I can tell, it would require forward declaration of the record and I know there are good reasons why that isn't possible. How would you envision forward declaration of a record method would look?

Agree. But your wise words are missing the point that the value of GetLastError is irrelevant in the context. Being curious about the value is pointless. Those that are curious about it regardless could just examine the source, and when that turns up no clue, trace though CreateThread in a kernel debugger, but in the end it will amount to nothing because the value [drum roll] doesn't matter.

You are wasting both your own and our time but as usual here the discussion continues ad nauseam even though the question has been resolved in the very first answer to the original post. I'm not sure how to explain it in a way that we haven't already tried. The value is meaningless, undocumented, random, forget about it ffs! If you really want an answer to this question, I suggest you ask it on stackexchange. I'm sure it will get the answers it deserves... Add a link back to this discussion for bonus points.

I think you did. Since the value of GetLastError is meaningless the way you used it: It doesn't matter and It doesn't matter

AFAIK it isn't and there's no reason for you to do it yourself because the value is irrelevant unless you're testing the result of an API function that has indicated that GetLastError should be used to get information about a failure. Here's another example of the exact same mistake: CreateThread() // GetLastError() returns 87 You don't need to "clear LastError" unless your function is using GetLastError as it's own status reporting mechanism and that would be very rare.

Probably not. The interpretation would depend on the API function that set the status value (notice I'm not calling it an error code). For example it could mean that a supplied parameter value didn't apply to the current context. As long as the caller handles the condition somehow then everything is fine.

I agree. The test case is invalid. Nope. There's no API contract that promises you that GetLastError will or should be zero at the point where you are testing it. If it's important to you that it is zero then sure, go ahead and set it to zero but it would be better to not misuse GetLastError that way. You should test GetLastError right after you have made an Win API call because at that point it will have been set to a relevant value.

Um... you're responding to a post which is over a year old, written by a person who's no longer alive...

Give it a rest, will you?

This is not related to your problem, but you should PostQuitMessage if you remove a WM_QUIT from the message queue.

Tools > Options > Environment Options > Environment Variables

No.

There are a lot of things that can cause this. Probably something you have running on your system. Might even be the debugger. https://devblogs.microsoft.com/oldnewthing/20160616-00/?p=93685 https://devblogs.microsoft.com/oldnewthing/20031001-00/?p=42343