Tag some char arrays with __attribute__((__nonstring__)) #871
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
Rebased with master |
Contributor
|
Hi @ac000, The patches look good. How do you install gcc-15? Do you build it from sources? gcc-15 is not released yet. |
Member
Author
|
Hi @xeioex I was testing on Fedora Rawhide. Fedora 42 is due out on Tuesday and will include GCC 15 (or at least a snapshot of 15.1)... However building GCC from source is not hard, this is what I do $ wget http://www.mirrorservice.org/sites/sourceware.org/pub/gcc/releases/gcc-x.x.x/gcc-x.x.x.tar.xz
$ tar -xf gcc-x.x.x.tar.xz
$ cd gcc-x.x.x
$ ./contrib/download_prerequisites
$ cd ..
$ mkdir gcc-x.x.x.build
$ cd gcc-x.x.x.build
$ ../gcc-x.x.x/configure --prefix=/opt/gcc-x.x.x --disable-multilib && make -j3 && echo "BUILD OK. Now INSTALL"
$ sudo make installEDIT: Latest GCC 15 snapshot is available here. |
Contributor
|
Is the patch ready for review? |
Member
Author
|
Sure, let me mark it as such... |
xeioex
requested changes
Apr 12, 2025
Contributor
There was a problem hiding this comment.
auto/clang: Add a NJS_NONSTRING macro.
- we use in this repo nginx-like style.
- do we need the article here? isn't NJS_NONSTRING unique?
auto/clang: added NJS_NONSTRING macro.
Please, also fix QuickJS parts as well.
To build with QuickJS
- build QuickJS
git clone https://github.com/bellard/quickjs
cd quickjs
CFLAGS='-fPIC' make libquickjs.a
./configure --cc-opt='-I/path/to/quickjs' --ld-opt='-L/path/to/quickjs' && make njs
places I found
diff --git a/external/qjs_query_string_module.c b/external/qjs_query_string_module.c
index bb787229..1d8c637f 100644
--- a/external/qjs_query_string_module.c
+++ b/external/qjs_query_string_module.c
@@ -537,7 +537,7 @@ qjs_string_encode(const uint32_t *escape, size_t size, const u_char *src,
u_char *dst)
{
uint8_t byte;
- static const u_char hex[16] = "0123456789ABCDEF";
+ static const u_char hex[16] NJS_NONSTRING = "0123456789ABCDEF";
do {
byte = *src++;
diff --git a/src/qjs_buffer.c b/src/qjs_buffer.c
index a45f57ce..a5cbf4c6 100644
--- a/src/qjs_buffer.c
+++ b/src/qjs_buffer.c
@@ -2354,7 +2354,7 @@ qjs_hex_encode(JSContext *ctx, const njs_str_t *src, njs_str_t *dst)
size_t i, len;
const u_char *start;
- static const u_char hex[16] = "0123456789abcdef";
+ static const u_char hex[16] NJS_NONSTRING = "0123456789abcdef";
len = src->length;
start = src->start;
Contributor
|
Also, after the patches gcc-15 is still failing to build. But on top of this work it succeeds. |
Member
Author
You mean using past-tense rather than the normal imperative mood? I detest it, but if you insist... however there are at least
I'll take a look. |
Member
Author
master with these patches plus your quickjs patch builds cleanly with GCC 15 for me (with QJS enabled). What warning/error are you seeing? |
Contributor
|
Contributor
Yes, the past tense. I agree that for new projects something like conventional commits should be used, but I would like to stick to naming convention user in this project for consistency.
yes, sometime it percolates. |
Member
Author
|
I remember now... these patches were done with a view to the atomic keys work having been merged where a problematic array is removed. I can either re-spin these patches to fully work with master, or wait until the atomic keys patches are in, keeping in mind GCC 15 will be in one popular distribution from Tuesday... so if it will be a while before that's merged then perhaps I should re-spin... |
Contributor
|
I as suggested, it makes sense to merge your patches after atomic strings. We plan to merge the atoms in the next release. |
ac000
force-pushed
the
wusi
branch
2 times, most recently
from
f97a2c4 to
d7db165
Compare
Member
Author
NOTE: This doesn't include the tagging of the |
Member
Author
|
xeioex
approved these changes
Apr 21, 2025
This is a wrapper around __attribute__((nonstring)). Traditionally this was used to mark char array variables that intentionally lacked a terminating NUL byte, this would then cause warnings to either be quelled or emitted for various memory/string functions. GCC 15 introduced a new warning, Wunterminated-string-initialization, which will always warn on things like static const char hex[16] = "0123456789ABCDEF"; However this is very much intentionally not NUL terminated. The above attribute also quells this new warning (which is also enabled by -Wextra). So the above example would become static const char hex[16] NJS_NONSTRING = "0123456789ABCDEF"; Link: <https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=44c9403ed1833ae71a59e84f9e37af3182be0df5> Link: <https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=622968990beee7499e951590258363545b4a3b57> Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178#c21> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In njs we have a number of character arrays which are intentionally not
NUL terminated.
With GCC 15 this
static const u_char hex[16] = "0123456789ABCDEF";
will trigger a warning (which we treat as an error) like
src/njs_string.h: In function ‘njs_string_encode’:
src/njs_string.h:212:36: error: initializer-string for array of ‘unsigned char’ truncates NUL terminator but destination lacks ‘nonstring’ attribute (17 chars into 16 available) [-Werror=unterminated-string-initialization]
212 | static const u_char hex[16] = "0123456789ABCDEF";
| ^~~~~~~~~~~~~~~~~~
By adding NJS_NONSTRING like
static const u_char hex[16] NJS_NONSTRING = "0123456789ABCDEF";
we no longer get the warning.
Closes: nginx#857
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Member
Author
|
Contributor
|
BTW, let's consider the following alternative approach from QuickJS. We can simply remove the array storage size to avoid the warning. This looks like even simpler and concise. |
Member
Author
|
Heh, well, yes, obviously you can do that. Or perhaps more likely const char hex[] = "0123456789ABCDEF";As they are different things. But then that changes the dynamics somewhat. You now have what looks like a normal NUL-terminated string literal. When what this really is, is a lookup table that does not need to be NUL-terminated. There was a reason it was declared (well I assume someone had thought about it...) const char hex[16] = "0123456789ABCDEF";
static const char wave_hdr[4] = "RIFF";IMO this is the right way to go, it nicely documents via code what the intended purpose of these things are. Or indeed where having the terminating NUL-byte would just complicate matters. |
Contributor
I do not think that introduction of gcc specific attributes is justified to save 1-byte. I think we should introduce the support for custom attributes only if we have to.
In principle, it confusing places, yes, this can be a thing. But in all the fixed places the intention is quite obvious and simple. |
Member
Author
|
Then let me ask this. Why was this declared like this in the first place? static const u_char hex[16] = "0123456789ABCDEF";It would of course just been simpler to not specify the size. (The answer of course is, this is not a string). |
Member
Author
|
Look at it another way. Without the attribute, you have two options
static const char hex[] = "0123456789ABCDEF";
char c = hex[33 % sizeof(hex)];Which could lead to undesirable consequences. Or you mutate it a different way which makes it unreadable... Neither of which are particularly appealing. The attribute gives you the best of both worlds... for me it's a no-brainer. |
|
@ac000 as an outsider to the project that just ran into this issue on latest GCC, i'm of the opinion that random non-standard custom attributes are weird and unweildy, so given how things have been just fine with the way things are for this long, I see the warning as the thing that is pointless and what should be disabled |
Member
Author
The thing is, the warning can help catch silly mistakes and anything we can do to help the compiler help us is a Good Thing(tm). Unless you've been living under a rock, I'm sure you're aware of the amount of hate C gets these days, anything we can do to counter that can only be good, but even if that wasn't the case... So, no, I'm not a fan of disabling compiler warnings (unless there really is very good reason or no other option). We did temporarily disable it in NGINX Unit, but then re-enabled it when the patches to allow the
Non-standard?, possibly, though clang and gcc do have at least some in common.
Hmm, beauty is in the eye of the beholder as they say. Is static const HEX[16] NJS_NONSTRING = "0123456789ABCDEF";
static const hex[16] __nonstring = "0123456789abcdef";But to be fair, I don't think either is particularly too unwieldy. And whether you like this attribute or attributes in general or not, it's the mechanism we currently have... |
Contributor
In principle, I can imagine where the |
Contributor
Member
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GCC 15 implements a new warning
-Wunterminated-string-initializationthat is also enabled by-Wextrathat we enable.This causes compilation failures (due to -Werror) due to the likes of
E.g.
These are very much meant not to be NUL terminated.
Now we could just disable this new warning. But I think it is worth leaving it enabled (the GCC developers also obviously feel it's useful enough to enable under -Wexta), anything that helps the compiler help us avoid silly mistakes is a good thing(tm), particularly in the current climate.
So rather than disable this warning, we can make use of the GCC "nonstring" variable attribute
__attribute__((__nonstring__)).This attribute is used to mark character arrays that are intentionally not NUL terminated.
So the above example would become (we of course wrap it in a more friendly name
NJS_NONSTRING)The first commit adds a wrapper around
__attribute__((__nonstring__))The second commit tags various character arrays with this attribute.
Note: This was done with an eye towards the atomic strings work which removes the need for one of the attribute tags.