Revert "fix #14873 properly by skipping abi field in importc type"

[Araq]

Reverts #17944

[timotheecour]

@Araq @arnetheduck

The ABI fields were added because they make certain code work that previously didn't, particularily such code that is careful about memory allocation - this PR breaks such code, and should ideally be rolled back.

that's too vague to justify a revert, what code specifically breaks? Note that #17944 fixes code that was broken because of those abi fields.

That message also doesn't address the points in the PR:

• why single out those types to have an abi field, whereas others don't
• why is the abi field correct size on some platforms but not others (eg osx)
• the abi fields don't guarantee correctness, because {.completeStruct.} (or {.sizeof: N.}, {.alignof: N.}) isn't used, which at least would add static_assert abi checks during c compilation phase
• why not use an alternative approach that doesn't involve manually writing the abi fields on each platform, which is very fragile (doesn't work on some major platforms like osx, and also what about alternative libc/compiler on platforms where it does given an abi field?)

I suggest writing an RFC to discuss this as well as what exactly breaks (reproducible, minimized code) and discussing alternative options before reverting this.

[Araq]

I suggest writing an RFC to discuss this as well as what exactly breaks (reproducible, minimized code) and discussing alternative options before reverting this.

We can and should do it differently after such an RFC has been accepted...

[arnetheduck]

what code specifically breaks

code that uses the sizes and offsets at compile time - for example memory pools and serialization libraries - I'm not sure how to be more specific - it also breaks nlvm but that's less of an issue - nlvm already maintains a Nim fork - but the point here is that just because something isn't part of the nim test suite or the important packages doesn't mean that it doesn't get used - a generally acceptable quality bar is that working code shouldn't change without observing the forms that come with breaking changes.

Removing the ABI markers also introduces risk of hard-to-detect bugs where the Nim and C compiler don't agree on the ABI which can't easily be detected until the application crashes - you're aware of this issue already though.

why single out those types to have an abi field, whereas others don't

The std lib has been growing organically and is larger than can be maintained at a consistent quality level - it's not surprising that it's not complete in this regard - if I were to pick a reason, most likely these types are the ones that actually have some unit test coverage.

the abi fields don't guarantee correctness

indeed they don't - -d:checkAbiSizes would be a good start to have enabled by default for the std lib tests to avoid such issues - that would be a change that doesn't break existing working code and makes it possible to use the std library more safely and productively.

why not use an alternative approach

to the best of my knowledge, there is none implemented today - when it gets implemented, it will be appropriate to remove the fields (in that order) - until then, typing them out is a pragmatic solution that works surprisingly well - fortunately, C ABI:s don't change usually, since C programmers tend to be intimately familiar with the difficulties that this causes in the C compilation model - once the ABI types have been typed out correctly, they become non-problems that don't really need alternative approaches.

In general, this is an example of how Nim at the Nim level must have some knowledge of the target environment it's compiling for in order to support systems programming features that exist in the language - this includes knowing details like the size of a float and a long, the alignment requirements of an MMX register and so on - without this, some Nim features simply can't work.

There have been some improvements suggested to deal with this - in particular, having a low-level libc wrapper that faithfully reproduces various C libraries out there with full ABI is a particularly simple and effective way to do it - #5698 shows how this could look - this in turn would give the Nim compiler enough information to meaningfully deal with the problem on the Nim side. The nice thing with these wrappers is that they can be generated automatically using for example nimterop - in the std library there's also support for generating some of this information automatically here - once that's been done, you no longer need C headers at all to use a library - this makes it significantly easier to distribute nim programs.

I suggest writing an RFC to discuss

I would suggest that as a general approach, the onus of writing an RFC would be on those that knowingly introduce breaking changes - we agree that such are sometimes necessary and an RFC is a good place to hash them out.

In this case, you've already analyzed yourself what code breaks - const size = sizeof(Lock) for example - since you know that user code breaks, the RFC process is appropriate - having to track the PR flow for breaking changes and play breakage police is not a sustainable, from the point of view of a significant Nim user. There are a few good litmus test questions to ask yourself:

• does this break user code?
• does this introduce compile-time vs runtime differences?
• can the Nim compiler reason about the code and give good error messages with this feature?
• does the change introduce potential for difficult runtime bugs in order to work around shallow issues that are noticed at compile time?

If the answer is yes to any of these questions, the std lib is not a good place for it most likely.

[timotheecour]

just replying on 1st point for now:

what code specifically breaks

code that uses the sizes and offsets at compile time

how so?
this gives a CT error both before and after #17944

when true:
  import std/locks
  let a = Lock.sizeof # ok
  const b = Lock.sizeof # CT error

again, can you please show minimized code that breaks?

[arnetheduck]

this gives a CT error both before and after #17944

You're right, apologies. I forgot that Nim prevents people from using these types with all features of the language, which means that the change will indeed not break existing working code. The way the fields are formed right now, it also doesn't make debugging worse, though the times that do correctly include all fields (even private ones) are somewhat easier to debug - ie the windows lock contains owner thread and counter - glibc likewise, if one were to dig.

It does prevent nlvm from working which uses this information for stack allocation, debug information and other causes. Upgrading nlvm will be harder as a result - it's currently at 1.2.x because we use it at Status.

An actual improvement here would be to make the ABI fields match libc exactly for the platforms where they are known - that would help debugging, printing and enable them to be used with the things I wrongly thought would break (systems programming).

[Araq]

I still like the custom $ for locks better anyway.

[timotheecour]

I still like the custom $ for locks better anyway.

that's not an argument in support of a revert though, this PR could've simply added back just the $ proc.

Since you've reverted #14873, you should re-open #14873 which was fixed by that PR...

Why can't nlvm use sizeof at RT instead of CT?
Debugging is still just as possible without the abi field, see #17944 (comment) for eg; and avoids all issues already mentioned earlier (eg abi lies on some platforms eg osx).

In future work, we can integrate cling and clang's libtooling to get the field at CT, which will allow full reflection (and JIT evaluation) in a robust way for client code that needs it, and it'll be 100% robust even for C++ code, unlike c2nim or nimterop.

[arnetheduck]

Why can't nlvm use sizeof at RT instead of CT?

To use any size at all, nlvm must know about it first - nlvm does not include a C compiler because doing so would add to its complexity - it is a much more simple and robust solution to simply type it out in Nim.

nlvm needs the information at compile because several features depend on knowing it at compile time, such as debug information, static analysis etc. Nim systems-level programs also benefit from knowing the size at compile time, in Nim - the proper solution here is to make sure the sizes are usable on the Nim side as well.

There's nothing fragile about putting the information in Nim - for the C library, the type sizes are part of the ABI in the vast majority of cases and cannot change on the C side, except when new platforms are added. There is very little complexity to this solution and it makes the std library easier to use on Nim with fewer moving parts, without more complex features that don't exist.

eg abi lies on some platforms eg osx

the proper fix here would be to add it on osx as well - as is, because of when it's not lying on osx, the library is simply incomplete in this regard.