public inbox for bzip2-devel@sourceware.org
 help / color / mirror / Atom feed
* Declare bzerrorstrings[] to be a const object.
@ 2022-04-26 14:33 Carlo Bramini
  2022-04-26 19:35 ` Mark Wielaard
  0 siblings, 1 reply; 3+ messages in thread
From: Carlo Bramini @ 2022-04-26 14:33 UTC (permalink / raw)
  To: bzip2-devel

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

Hello,
by looking the content of the map file, I discovered that bzerrorstrings[] is not const.
So, I would like to suggest the tiny change into the attached patch, for declaring both bzerrorstrings[] and the addressed strings as const.
The patch has been created from the latest sources into the repository with GIT.

Sincerely,

Carlo Bramini.

[-- Attachment #2: bzlib.c.patch --]
[-- Type: application/octet-stream, Size: 337 bytes --]

diff --git a/bzlib.c b/bzlib.c
index 2178655..752d5a0 100644
--- a/bzlib.c
+++ b/bzlib.c
@@ -1536,7 +1536,7 @@ void BZ_API(BZ2_bzclose) (BZFILE* b)
 /*--
    return last error code 
 --*/
-static const char *bzerrorstrings[] = {
+static const char * const bzerrorstrings[] = {
        "OK"
       ,"SEQUENCE_ERROR"
       ,"PARAM_ERROR"

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Declare bzerrorstrings[] to be a const object.
  2022-04-26 14:33 Declare bzerrorstrings[] to be a const object Carlo Bramini
@ 2022-04-26 19:35 ` Mark Wielaard
  2022-04-27  9:21   ` Carlo Bramini
  0 siblings, 1 reply; 3+ messages in thread
From: Mark Wielaard @ 2022-04-26 19:35 UTC (permalink / raw)
  To: Carlo Bramini; +Cc: bzip2-devel

Hi,

On Tue, Apr 26, 2022 at 04:33:22PM +0200, Carlo Bramini via Bzip2-devel wrote:
> by looking the content of the map file, I discovered that bzerrorstrings[] is not const.

Which map file is this?

> So, I would like to suggest the tiny change into the attached patch, for declaring both bzerrorstrings[] and the addressed strings as const.
> The patch has been created from the latest sources into the repository with GIT.

Although the patch seems fine I must admit that I don't fully
understand why it is necessary. Isn't an array (name) not always
constant? You cannot assign anything to an array name since it is a
contant pointer to the first element. Or is that not what this "const"
is about?

Thanks,

Mark


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Declare bzerrorstrings[] to be a const object.
  2022-04-26 19:35 ` Mark Wielaard
@ 2022-04-27  9:21   ` Carlo Bramini
  0 siblings, 0 replies; 3+ messages in thread
From: Carlo Bramini @ 2022-04-27  9:21 UTC (permalink / raw)
  To: bzip2-devel

Hello,
I compiled BZip2 for an embedded system, to be used in an homebrew project.
The platform is an ARM Cortex M4 microcontroller which provides some internal memories, like all other devices with similar purpose: 
* a FLASH memory, which is usually bigger and it stores the code and constant data of a firmware.
* a RAM memory, which is tipically much smaller than the FLASH and it is used for the stack and all data that can change their value during the execution of the program.

Since the RAM memory is a limited resource, one of the tasks of the developer is to move all data that won't change their value into the FLASH, not only for security reasons but because it is needed to do so, for maximizing the amount of available RAM.

To do so, the developer usually examines a map file to see if some memory could be optimized, especially when using code written by others.

So, I gave a look to the map file and I have seen that something could be improved in this way.

In this specific case, the bzerrorstrings[] is actually made by two distinct logical objects:
1) the bzerrorstrings[], which holds some pointers to strings.
2) the strings pointed by the bzerrorstrings[].

Actually, you can decide which of these items is a const object: the strings, the bzerrorstrings[] table or both.

The attached patch just adds another const attribute for having both items into the read-only area, which seems to be the expected behaviour of that piece of code.

It is true that this fix won't improve the memory usage on a system like a PC, however it could be also useful since an exception could be raised if something will accidentally write into a memory expected to be read-only.

By looking my map file, I have to say that there are also other things that may be declared as const:

blocksort.c:
- static Int32 incs[14]
bzip2.c
- const Char* zSuffix[BZ_N_SUFFIX_PAIRS]
- const Char* unzSuffix[BZ_N_SUFFIX_PAIRS]
crctable.c
- UInt32 BZ2_crc32Table[256]
randtable.c
- Int32 BZ2_rNums[512]

but unfortunately I'm unsure about the code, so the bzerrorstrings[] table was the easiest one for me to understand that it was a safe change.

I hope that you will find this report useful.
Sincerely,

Carlo Bramini.


> Il 26/04/2022 21:35 Mark Wielaard <mark@klomp.org> ha scritto:
> 
>  
> Hi,
> 
> On Tue, Apr 26, 2022 at 04:33:22PM +0200, Carlo Bramini via Bzip2-devel wrote:
> > by looking the content of the map file, I discovered that bzerrorstrings[] is not const.
> 
> Which map file is this?
> 
> > So, I would like to suggest the tiny change into the attached patch, for declaring both bzerrorstrings[] and the addressed strings as const.
> > The patch has been created from the latest sources into the repository with GIT.
> 
> Although the patch seems fine I must admit that I don't fully
> understand why it is necessary. Isn't an array (name) not always
> constant? You cannot assign anything to an array name since it is a
> contant pointer to the first element. Or is that not what this "const"
> is about?
> 
> Thanks,
> 
> Mark

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-04-27  9:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-26 14:33 Declare bzerrorstrings[] to be a const object Carlo Bramini
2022-04-26 19:35 ` Mark Wielaard
2022-04-27  9:21   ` Carlo Bramini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).