Skip to content

Commit

Permalink
Make attributes preceed function declarations
Browse files Browse the repository at this point in the history 
This also adds soem additional ATTR_ONNULL and ATTR_RETNONNULL markers
that were absent previously.

The order for attributes is roughly:
noreturn, format, nonnull, returns_nonnull, malloc, alloc_size, access
  • Loading branch information
@BenBE
BenBE committed Jul 11, 2024
1 parent 0720b10 commit 398fb30
Showing 1 changed file with 36 additions and 27 deletions.
63 changes: 36 additions & 27 deletions XUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,26 @@ in the source distribution for its full text.
#include "Macros.h"


void fail(void) ATTR_NORETURN;
ATTR_NORETURN
void fail(void);

void* xMalloc(size_t size) ATTR_ALLOC_SIZE1(1) ATTR_MALLOC;
ATTR_RETNONNULL ATTR_MALLOC ATTR_ALLOC_SIZE1(1)
void* xMalloc(size_t size);

void* xMallocArray(size_t nmemb, size_t size) ATTR_ALLOC_SIZE2(1, 2) ATTR_MALLOC;
ATTR_RETNONNULL ATTR_MALLOC ATTR_ALLOC_SIZE2(1, 2)
void* xMallocArray(size_t nmemb, size_t size);

void* xCalloc(size_t nmemb, size_t size) ATTR_ALLOC_SIZE2(1, 2) ATTR_MALLOC;
ATTR_RETNONNULL ATTR_MALLOC ATTR_ALLOC_SIZE2(1, 2)
void* xCalloc(size_t nmemb, size_t size);

void* xRealloc(void* ptr, size_t size) ATTR_ALLOC_SIZE1(2);
ATTR_RETNONNULL ATTR_ALLOC_SIZE1(2)
void* xRealloc(void* ptr, size_t size);

void* xReallocArray(void* ptr, size_t nmemb, size_t size) ATTR_ALLOC_SIZE2(2, 3);
ATTR_RETNONNULL ATTR_ALLOC_SIZE2(2, 3)
void* xReallocArray(void* ptr, size_t nmemb, size_t size);

void* xReallocArrayZero(void* ptr, size_t prevmemb, size_t newmemb, size_t size) ATTR_ALLOC_SIZE2(3, 4);
ATTR_RETNONNULL ATTR_ALLOC_SIZE2(3, 4)
void* xReallocArrayZero(void* ptr, size_t prevmemb, size_t newmemb, size_t size);

/*
* String_startsWith gives better performance if strlen(match) can be computed
Expand Down Expand Up @@ -64,21 +71,21 @@ static inline bool String_eq_nullable(const char* s1, const char* s2) {
return false;
}

ATTR_NONNULL
char* String_cat(const char* s1, const char* s2) ATTR_MALLOC;
ATTR_NONNULL ATTR_RETNONNULL ATTR_MALLOC
char* String_cat(const char* s1, const char* s2);

ATTR_NONNULL
char* String_trim(const char* in) ATTR_MALLOC;
ATTR_NONNULL ATTR_RETNONNULL ATTR_MALLOC
char* String_trim(const char* in);

ATTR_NONNULL_N(1)
ATTR_NONNULL_N(1) ATTR_RETNONNULL
char** String_split(const char* s, char sep, size_t* n);

void String_freeArray(char** s);

ATTR_NONNULL
char* String_readLine(FILE* fp) ATTR_MALLOC;
ATTR_NONNULL ATTR_MALLOC
char* String_readLine(FILE* fp);

ATTR_NONNULL
ATTR_NONNULL ATTR_RETNONNULL
static inline char* String_strchrnul(const char* s, int c) {
#ifdef HAVE_STRCHRNUL
return strchrnul(s, c);
Expand All @@ -91,32 +98,33 @@ static inline char* String_strchrnul(const char* s, int c) {
}

/* Always null-terminates dest. Caller must pass a strictly positive size. */
ATTR_ACCESS3_W(1, 3)
ATTR_ACCESS3_R(2, 3)
ATTR_NONNULL ATTR_ACCESS3_W(1, 3) ATTR_ACCESS3_R(2, 3)
size_t String_safeStrncpy(char* restrict dest, const char* restrict src, size_t size);

ATTR_FORMAT(printf, 2, 3)
ATTR_NONNULL_N(1, 2)
ATTR_FORMAT(printf, 2, 3) ATTR_NONNULL_N(1, 2)
int xAsprintf(char** strp, const char* fmt, ...);

ATTR_FORMAT(printf, 3, 4)
ATTR_ACCESS3_W(1, 2)
ATTR_FORMAT(printf, 3, 4) ATTR_NONNULL_N(1, 3) ATTR_ACCESS3_W(1, 2)
int xSnprintf(char* buf, size_t len, const char* fmt, ...);

char* xStrdup(const char* str) ATTR_NONNULL ATTR_MALLOC;
ATTR_NONNULL ATTR_RETNONNULL ATTR_MALLOC
char* xStrdup(const char* str);

ATTR_NONNULL
void free_and_xStrdup(char** ptr, const char* str);

ATTR_ACCESS3_R(1, 2)
char* xStrndup(const char* str, size_t len) ATTR_NONNULL ATTR_MALLOC;
ATTR_NONNULL ATTR_RETNONNULL ATTR_MALLOC ATTR_ACCESS3_R(1, 2)
char* xStrndup(const char* str, size_t len);

ATTR_ACCESS3_W(2, 3)
ATTR_NONNULL ATTR_ACCESS3_W(2, 3)
ssize_t xReadfile(const char* pathname, void* buffer, size_t count);
ATTR_ACCESS3_W(3, 4)
ATTR_NONNULL ATTR_ACCESS3_W(3, 4)
ssize_t xReadfileat(openat_arg_t dirfd, const char* pathname, void* buffer, size_t count);

ATTR_ACCESS3_R(2, 3)
ATTR_NONNULL ATTR_ACCESS3_R(2, 3)
ssize_t full_write(int fd, const void* buf, size_t count);

ATTR_NONNULL
static inline ssize_t full_write_str(int fd, const char* str) {
return full_write(fd, str, strlen(str));
}
Expand All @@ -129,6 +137,7 @@ int compareRealNumbers(double a, double b);
/* Computes the sum of all positive floating point values in an array.
NaN values in the array are skipped. The returned sum will always be
nonnegative. */
ATTR_NONNULL ATTR_ACCESS3_R(1, 2)
double sumPositiveValues(const double* array, size_t count);

/* Returns the number of trailing zero bits */
Expand Down

9 comments on commit 398fb30

@Explorer09
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenBE May I suggest to remove some of the nonnull attributes on these functions?
Some of the nonnull attributes added in this commit 398fb30 might look too strict for what the function was designed.

  • full_write - I suggest to remove ATTR_NONNULL and just leave ATTR_ACCESS3_R(2, 3) in place. The function was designed that a zero-size write is allowed (it just does nothing), and that ATTR_ACCESS3_R(2, 3) would be sufficient for the sanity check.
  • The above doesn't apply to full_write_str. That is, full_write_str should keep ATTR_NONNULL.
  • sumPositiveValues - I suggest to remove ATTR_NONNULL and just leave ATTR_ACCESS3_R(1, 2) in place. When I wrote this sumPositiveValues function, I expected count argument being zero is valid use. ATTR_ACCESS3_R(1, 2) would be enough for the sanity check.

@BenBE
Copy link
Member Author

@BenBE BenBE commented on 398fb30 Jul 21, 2024

Choose a reason for hiding this comment

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

With both full_write and sumPositionalValues you still can have the size/count be specified as 0. The only limitation which ATTR_NONNULL has on the function declaration is forcing all provided pointers to be non-NULL. You still can do full_write(file, some_buffer, 0); as long as some_buffer points to a valid memory address. The implication of the ATTR_ACCESS_R(2,3) alone would be that I could do full_write(NULL, NULL, 0); which is wrong in one instance (the file argument should be non-NULL, always) and surprising on a second front (the buffer is NULL, which won't matter because it should never be accessed). Having the ATTR_NONNULL here avoids both cases. One could argue re full_write(file, NULL, 0) but IMHO that's surprising and at least inviting to cause errors in other parts of the code.

@Explorer09
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenBE Well, the first argument of full_write is an int type FD, not a pointer type, so the ATTR_NONNULL can't apply to it. That only leaves the second buffer argument which ATTR_NONNULL can apply to.

My concern is that the buffer and size arguments might come from objects or structures whose members are left uninitialized (NULL pointers and zero size) for any reason. If we apply ATTR_NONNULL, we risk letting compiler over-optimize things (e.g. with code that always de-reference the pointer while it's not safe to do so).

For sumPositiveValues in particular, I did anticipate sumPositiveValues(NULL, 0) can happen, for example in this code:

static double Meter_computeSum(const Meter* this) {
   double sum = sumPositiveValues(this->values, this->curItems);
   return MINIMUM(DBL_MAX, sum);
}

You may argue that the meters without numeric items to draw all have their "Bar" display mode disabled, but I am trying to be cautious here, and don't like the function to be optimized with a stricter constraint that I didn't intend to have at the beginning.

@BenBE
Copy link
Member Author

@BenBE BenBE commented on 398fb30 Jul 21, 2024

Choose a reason for hiding this comment

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

ATTR_NONNULL is not only about optimization, but also on constraint checking. If you can have a NULL passed in which the caller doesn't check, the compiler will usually tell you about this constraint violation (I even had the compiler produce multi-page diagnostics about a possible NULL de-reference across compilation unit boundaries purely based on the non-NULL attribute of one function argument). Yes, it won't do this reliably in all cases and also not if you don't do global optimizations, but even then having the attributes marked rather consistently will provide you with quite a lot of benefits.

In the end, ATTR_NONNULL is about the contract to the caller and declaring "I don't want you to pass a NULL value here" is a definite contract to the caller which when violating causes undefined behaviour (which may result in a crash).

So if your caller in Meter_computeSum passes that value this->values to sumPositiveValues without ensuring a non-NULL value, that's a violation from the caller.

FWIW, if you involve undefined behaviour, don't expect the compiler to do anything in particular. For all intents and purposes, if the compiler were to decide the undefined behaviour could best be realized by formatting your hard-drive, this would fully be correct by the definition of the C standard; just probably not what you intended for your code to do.

Or to summarize: If you aren't sure, you always pass non-NULL pointers along, check them before passing them. If the compiler notices they could not have been NULL (because of another ATTR_NONNULL in effect) it will tell you. Try to be defensive with your code; and if you can check for non-NULLs automatically with the compiler then that's a benefit you should embrace.

@Explorer09
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenBE I mean that passing this->values to sumPositiveValues when this->values is NULL should be allowed as long as this->curItems is zero. The only constraint for the Meter_computeSum function mentioned is this must be non-null. With ATTR_NONNULL added to sumPositiveValues, it could break that use case by turning an allowed behavior into an undefined one.

@BenBE
Copy link
Member Author

@BenBE BenBE commented on 398fb30 Jul 22, 2024

Choose a reason for hiding this comment

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

And my stand on this is that Meter_computeSum is responsible for proper checking of arguments and sumPositiveValues does not accept NULLs and thus should never see them …

@Explorer09
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenBE That would be additional code than I initially intended.

The code would become this...

static double Meter_computeSum(const Meter* this) {
   double sum = this->values ? sumPositiveValues(this->values, this->curItems) : 0.0;
   // Prevent rounding to infinity in IEEE 754
   return MINIMUM(DBL_MAX, sum);
}

While for the original sumPositiveValues code, it should be fine for the call like sumPositiveValues(NULL, 0), in which case it returns 0.0:

double sumPositiveValues(const double* array, size_t count) {
   double sum = 0.0;
   for (size_t i = 0; i < count; i++) {
      if (isPositive(array[i])) {
         sum += array[i];
      }
   }
   return sum;
}

@BenBE
Copy link
Member Author

@BenBE BenBE commented on 398fb30 Jul 22, 2024

Choose a reason for hiding this comment

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

Or simply:

static double Meter_computeSum(const Meter* this) {
   if (!this->values || !this->curItems)
      return 0.0;
   double sum = sumPositiveValues(this->values, this->curItems);
   // Prevent rounding to infinity in IEEE 754
   return MINIMUM(DBL_MAX, sum);
}

@Explorer09
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenBE For code size, I would rather not do that redundant this->curItems check and just go with this:

static double Meter_computeSum(const Meter* this) {
   if (!this->values)
      return 0.0;
   double sum = sumPositiveValues(this->values, this->curItems);
   // Prevent rounding to infinity in IEEE 754
   return MINIMUM(DBL_MAX, sum);
}

Please sign in to comment.