Tag some char arrays with __attribute__((__nonstring__)) #871
Conversation
This file contains 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
|
Rebased with master |
|
Hi @ac000, The patches look good. How do you install gcc-15? Do you build it from sources? gcc-15 is not released yet. |
|
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. |
|
Is the patch ready for review? |
|
Sure, let me mark it as such... |
xeioex
requested changes
Apr 12, 2025
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;
|
Also, after the patches gcc-15 is still failing to build. But on top of this work it succeeds. |
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. |
master with these patches plus your quickjs patch builds cleanly with GCC 15 for me (with QJS enabled). What warning/error are you seeing? |
|
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. |
|
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... |
|
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
NOTE: This doesn't include the tagging of the |
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 <[email protected]>
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 <[email protected]>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.