Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

followup #17944, remove other abi fields #17971

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 8, 2021

followup #17944

question for reviewer (not blocking this PR though): why bycopy only for one of the C_JmpBuf declarations? what was the reason for bycopy here?

future work

  • this gives me an idea: we could report sizeof, alignof in nim doc for all types (importc in particular but not limited to), which can be computed at RT; it's platform specific, but then again, we can also run nim doc on a per-platform basis if needed
  • consider also removing other hidden padding fields, eg:
lib/posix/posix_linux_amd64.nim:122:5:    pad: array[4, clong]
lib/posix/posix_linux_amd64.nim:201:5:    pad1: cshort
lib/posix/posix_linux_amd64.nim:203:5:    pad2: cshort
lib/posix/posix_linux_amd64.nim:215:5:    pad0 {.importc: "__pad0".}: cint
lib/posix/posix_linux_amd64.nim:321:5:    pad {.importc: "_pad"}: array[128 - 56, uint8]
lib/posix/posix_linux_amd64.nim:384:7:      pad: array[16, cint]
lib/posix/posix_linux_amd64.nim:391:7:      pad: array[16, cint]
lib/posix/posix_linux_amd64.nim:414:8:    ss_padding: array[128 - sizeof(cshort) - sizeof(culong), char]
lib/posix/posix_nintendoswitch.nim:191:7:      pad1: clong
lib/posix/posix_nintendoswitch.nim:193:7:      pad2: clong
lib/posix/posix_nintendoswitch.nim:195:7:      pad3: clong
lib/posix/posix_nintendoswitch.nim:198:7:      pad1: clong
lib/posix/posix_nintendoswitch.nim:200:7:      pad2: clong
lib/posix/posix_nintendoswitch.nim:202:7:      pad3: clong
lib/posix/posix_nintendoswitch.nim:345:8:    ss_padding1: array[64 - sizeof(cuchar) - sizeof(cshort), char]

@Araq
Copy link
Member

Araq commented May 11, 2021

@arnetheduck what exactly are the implications for nlvm?

@arnetheduck
Copy link
Contributor

nlvm needs to know the exact size of things to allocate enough stack memory for them, and later to do pointer/offset computations on them, and indeed to generate correct debug information. It cannot (pragmatically) read this information from the C header.

Nim sometimes can cheat and delegate this to the C compiler but gets into trouble as well in some specific cases where pointer arithmetic, sizeof etc is used - at the end of the day, if Nim and C disagree on struct size you get subtle crashes when least expecting them - the effects in nlvm are just more obvious and easy to spot - put another way, not having accurate size (and alignment) information prevents certain nim programs from being written, specially on the systems end of things (where we operate often), regardless which backend you end up using.

What practically happens is that Nim passes a memory location with the wrong number of bytes allocated to a C function that subsequently smashes past the size and overwrites the next variable.

Removing the ABI fields with poor or no understanding the consequences will break working code.

As to bycopy, I remember there was some weird reason, but I can't remember what it was - probably some function declaration that was inaccurate but changing it would break backwards compat so bycopy was used as a workaround.

@ringabout
Copy link
Member

Since #17944 was reverted, close this PR for now.

@ringabout ringabout closed this Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants