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

Fix bugzilla 23812 - ImportC: allow adding function attributes to imp… #16820

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

tim-dlang
Copy link
Contributor

@tim-dlang tim-dlang commented Aug 29, 2024

…orted C functions

This adds a new pragma for ImportC, which allows to set default storage classes. Only nothrow, @nogc and pure are supported for now. They can be disabled later with the same pragma using a minus using #pragma D pop #pragma attribute(pop).

Unknown pragmas are ignored.

The pragma starts with identifier D to avoid conflicts.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @tim-dlang! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
23812 enhancement ImportC: allow adding function attributes to imported C functions

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#16820"

Comment on lines 14 to 24
```
#pragma D nogc nothrow
#include <somelibrary.h>
```

Default storage classes can be disabled again by adding a minus:
```
#pragma D -nogc -nothrow
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not pragma D push(nogc, nothrow) and pop? Which fits better with pragmas of other C compiler vendors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would pop just remove nothrow in that example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would pop just remove nothrow in that example?

Using pop now reverses the last push, which can contain multiple storage classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, so you could do push(nothrow) and now the situation is nothrow, but not nogc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this use case would be more complicated. The pragma could be combined with the original idea of using minus, like this: #pragma D push(-nothrow). But I'm not sure, how common this would be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be the most common case to default to nothrow, nogc in C code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, my use case is handled by any of these mechanisms. Having something is better than nothing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be the most common case to default to nothrow, nogc in C code.

The obvious exception being functions that do callbacks via a function-pointer parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added tests for callbacks. Function pointers get the same attributes as functions, which should cover most use cases.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Sep 2, 2024

cc @WalterBright

@schveiguy
Copy link
Member

I would love to see this merged...

@WalterBright
Copy link
Member

There are multiple extensions like this, and the only thing they share is inconsistency:

__declspec
__attribute__
__pragma
_Pragma
#pragma

Unfortunately, those only apply to a single declaration. The #pragma is the only one to apply to blocks of code, however, that can only be applied outside of macros. #pragma is awkward in that it can appear between any other two tokens.

But #pragma seems like the most pragmatic(!) approach. ImportC already supports:
https://learn.microsoft.com/en-us/cpp/preprocessor/pack?view=msvc-170

https://gcc.gnu.org/onlinedocs/gcc-4.4.4/gcc/Structure_002dPacking-Pragmas.html;

"#pragma pack(n) simply sets the new alignment.
#pragma pack() sets the alignment to the one that was in effect when compilation started
(see also command line option -fpack-struct[=<n>] see [Code Gen Options](https://gcc.gnu.org/onlinedocs/gcc-4.4.4/gcc/Code-Gen-Options.html#Code-Gen-Options)).
#pragma pack(push[,n]) pushes the current alignment setting on an internal stack and then optionally sets the new alignment.
#pragma pack(pop) restores the alignment setting to the one saved at
the top of the internal stack (and removes that stack entry). Note
that #pragma pack([n]) does not influence this internal stack;
thus it is possible to have #pragma pack(push) followed by
multiple #pragma pack(n) instances and finalized by a single #pragma pack(pop)."

Here we are adding a confusingly different syntax:

#pragma D push([storage classes...])

I suggest a consistent one would be:

#pragma attribute(push, [storage classes...])
etc.

It's not necessary to add the D qualifier.

@schveiguy
Copy link
Member

I suggest a consistent one would be:

All looks decent to me! As long as I can do:

#pragma attribute(push, nogc, nothrow);
#include <stdio.h>

Then I am happy!

@tim-dlang tim-dlang force-pushed the issue23812 branch 2 times, most recently from 9de583c to 1e81fb9 Compare September 10, 2024 16:19
@tim-dlang
Copy link
Contributor Author

I have implemented the new syntax:

#pragma attribute(push, nogc, nothrow)
#include <somelibrary.h>
#pragma attribute(pop)

The functionality is the same.

```
#pragma attribute(push, [storage classes...])
```
The storage classes `nothrow`, `nogc` and `pure` are supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that unrecognized attributes are ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Sep 11, 2024

One thing that I don't like about this is that you actually need to uglify your C code to support what should be the most common case on the D side. Why not simply implement this on the D side as an extension of the import statement? i.e.:

@nogc import cmodule : func;
pure import cmodule: gun;
pure nothrow @nogc import othercmodule;

That way we don't use ugly pragmas to modify C code. Note that this syntax should be restricted only to C modules.

@schveiguy
Copy link
Member

Why not simply implement this on the D side as an extension of the import statement?

When is printf ever pure or throwing or using the GC? Why should I always have to remember to put extra attributes when I import it? The file that defines the symbol should be the file that defines it's interface, not the importer.

What you ironically are suggesting is adding more C behavior to D, by configuring behavior before including the file.

Besides, this means the compiler has to re-parse the whole thing, and define a separate module for each time you import it. It's a much harder change, and leaves some really nasty corner cases.

@RazvanN7
Copy link
Contributor

When is printf ever pure or throwing or using the GC? Why should I always have to remember to put extra attributes when I import it? The file that defines the symbol should be the file that defines it's interface, not the importer.

This argument is kind of moot considering that you will need to annotate the functions also.

What you ironically are suggesting is adding more C behavior to D, by configuring behavior before including the file.

Well, you are importing C modules so the "adding more C behavior to D" ship has already sailed.

Besides, this means the compiler has to re-parse the whole thing, and define a separate module for each time you import it. It's a much harder change, and leaves some really nasty corner cases.

No, there are workarounds for that. A potential solution is have something along the lines of import cmodule: nogc s. That way, you parse the file a single time and whenever you see this sort of import you simply update the symbol's attribute list. Anyway, I'm sure we can come up with something that works.

I'm just in the boat that C code should be kept clean C code. If we open the door to annotating C code so that it works with D it feels like a lost battle.

…orted C functions

This adds a new pragma for ImportC, which allows to set default storage
classes. Only `nothrow`, `@nogc` and `pure` are supported for now.
They can be disabled later using `#pragma attribute(pop)`.

Unknown storage classes are ignored.
@tim-dlang
Copy link
Contributor Author

What you ironically are suggesting is adding more C behavior to D, by configuring behavior before including the file.

Well, you are importing C modules so the "adding more C behavior to D" ship has already sailed.

ImportC modules and D modules are currently very similar: The modules are independent of the context of the importing module. With your proposal they would depend on attributes of the import statement. This would be similar to C includes, where macros before the include can influence parts of a header.

Besides, this means the compiler has to re-parse the whole thing, and define a separate module for each time you import it. It's a much harder change, and leaves some really nasty corner cases.

No, there are workarounds for that. A potential solution is have something along the lines of import cmodule: nogc s. That way, you parse the file a single time and whenever you see this sort of import you simply update the symbol's attribute list. Anyway, I'm sure we can come up with something that works.

Updating the attributes of a symbol after it was already used in another module would be problematic. The compilation could then depend on the order of modules on the command line or if separate compilation is used.

An alternative would be a mixin, which redeclares all functions from an ImportC module with the correct attributes.

@WalterBright
Copy link
Member

@tim-dlang I agree. The importer must not be able to modify the importee, or the whole module system falls apart. The module import system must:

  1. be order independent
  2. be context independent

Another problem is there would be no control over the attributes applied to individual functions.

@WalterBright WalterBright merged commit 0e8e670 into dlang:master Sep 14, 2024
41 checks passed
@tim-dlang tim-dlang deleted the issue23812 branch September 14, 2024 17:59
@schveiguy
Copy link
Member

I'm just in the boat that C code should be kept clean C code.

So one thing about C code that D does not have is a really easy mechanism to keep your C code "clean", yet still change things globally -- with #include. You can make a new module that has the attributes you want, and import that. You don't have to change the original C.

For example, your use case:

@nogc import cmodule : func;
pure import cmodule: gun;

Can be done via:

// cmodule_nogc.c
pragma attribute(push, nogc)
#include "cmodule.c"
// cmodule_pure.c
pragma(attribute(push, pure)
#include "cmodule.c"
// d file
import cmodule_nogc : func;
import cmodule_pure : gun;

@WalterBright
Copy link
Member

@schveiguy yes, those can work

tim-dlang added a commit to tim-dlang/dlang.org that referenced this pull request Sep 15, 2024
The new pragma is implemented in dlang/dmd#16820.
The documentation is based on the changelog entry.
dlang-bot pushed a commit to dlang/dlang.org that referenced this pull request Sep 15, 2024
The new pragma is implemented in dlang/dmd#16820.
The documentation is based on the changelog entry.
thewilsonator pushed a commit to thewilsonator/dmd that referenced this pull request Oct 7, 2024
…orted C functions (dlang#16820)

This adds a new pragma for ImportC, which allows to set default storage
classes. Only `nothrow`, `@nogc` and `pure` are supported for now.
They can be disabled later using `#pragma attribute(pop)`.

Unknown storage classes are ignored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants