|
Thanks, that was indeed the problem. No idea why it was checked - I almost always use tags. 
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
|
|
|
|
|
Vaclav_ wrote: The error "points " to #define which is redefined in this class - temporary for test purposes.
Are you doing something like this?
#include "spi.h" // #defines DEVICE_SPI, etc
#define DEVICE_SPI_"dev/spidev0.0" // Temporary define
class C_VNA {
};
That would mean that you're re-defining DEVICE_SPI, but normally your compiler should warn about redifines. Maybe you need #undef where you set up your temp defines. e.g.
#ifdef DEVICE_SPI
#define ORIG_DEVICE_SPI DEVICE_SPI
#undef DEVICE_SPI
#endif
#define DEVICE_SPI "dev/spidev0.0"
#ifdef ORIG_DEVICE_SPI
#undef DEVICE_SPI
#define DEVICE_SPI ORIG_DEVICE_SPI
#undef ORIG_DEVICE_SPI
#endif
|
|
|
|
|
Inside a function, this declares a variable called i2c_ads1115 with type CLASS_I2CIO and calls the constructor with the two values DEVICE_I2C and ADDRESS_ADS:
CLASS_I2CIO i2c_ads1115(DEVICE_I2C,ADDRESS_ADS);
Inside a class definition it declares a member function called i2c_ads1115 that returns a CLASS_I2CIO type and takes two arguments, which is where the compiler is having problems. Instead of DEVICE_I2C being a type, you have a #defined string.
To get i2c_ads1115 as a member variable, declare it the same way as the i2c_ioctl member variable, then construct it with arguments in the initialization list of the C_VNA class.
|
|
|
|
|
Thanks, I'll give it a go.
One more question.
Why I have no issue with using same syntax making an instance of the class in main ()?
Stupid question - could I just change the #define to "include" type ?
|
|
|
|
|
I can't say why that syntax defines a variable using a constructor in one place and declares a member function in another - you would have to ask the people who designed C++ that.
Making the #define include the type would probably give you a different error message, but it would still be attempting to declare a member function.
|
|
|
|
|
The compiler expects a function signature, but the function arguments you specified are missing a type.
Try somthing like this
CLASS_I2CIO i2c_ads1115_test(const char* DEVICE_I2C, int ADDRESS_ADS);
(not sure what type the second argument is supposed to be)
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
|
|
|
|
|
Second argument function is an I2C device address = in this case A/D converter ADS1115.
|
|
|
|
|
Suppose I have a function that takes an array pointer and an index declared as type unsigned int and the function will access the array at that index.
AFAIK "Index < 0" should never evaluate to be true regardless of what the caller passed because the unsigned type will control how the bits at the address of Index are interpreted and there will be no interpretation of a bit as an indication of sign.
So implicitly an unsigned int is not capable of an access violation Below / before an array's first element given the pointer arithmetic. All that being said it does not seem at all necessary to test it.
There are other reasons to use the test however.
#1 it may improve readability as someone who reads the code sees that a full boundary check is being done
#2 it may make the code symmetric with other code that tests upper and lower boundaries of any other type of variable
#3 if the function is later modified and the type is accidentally changed to int (for example the types are changed when porting to a different machine architecture) or the code is copy and pasted to a different context where Index is not unsigned, omitting the bounds check introduces the possibility of an access violation
#4 I think it's possible, perhaps likely, that an intelligent compiler will optimize it out completely, even if it didn't we're not talking about a grave loss of efficiency.
So although I feel stupid writing what seems to be redundant and unnecessary code and concern myself with conciseness, for a robust application it seems worthwhile.
|
|
|
|
|
I would keep it simple: an unsigned cannot be less than zero, full stop.
|
|
|
|
|
CPallini wrote: I would keep it simple: an unsigned cannot be less than zero, full stop.
This simple fact seems to be no longer on the curriculum! Wrote a bit about it to the caller!
I feel that an awfull lot of skills are no longer taught, the same skills I had to learn myself, e.g. How does a computer actually work! Registers,Stackpointers, The Stack and the Heap, the function of the OS, Machine Code, what does an assembler do, what does a compiler do, etc. I will encourage anyone willing to study 'C' or 'CPP'. These people on this queery seem to be basically stuck on the underlying hardware concepts.
Kind Regards,
Bram van Kampen
|
|
|
|
|
Member 14088880 wrote:
So implicitly an unsigned int is not capable of an access violation Below / before an array's first element given the pointer arithmetic. All that being said it does not seem at all necessary to test it.
If your function looked something like:
void myfunction(int array[], unsigned int count)
{
for (unsigned x = 0; x < count; x++)
...
}Then it is possible to call it like:
myfunction(array, -1); which would result in the for() loop running for a very long time.
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"You can easily judge the character of a man by how he treats those who can do nothing for him." - James D. Miles
|
|
|
|
|
I'm genuinely surprised and my paranoia has been rewarded. I think I've misunderstood something fundamental. Here is a code sample:
#include <stdio.h>
void print_iterator(int integer_array[], unsigned int index)
{
unsigned int i;
if(index < 0)
{
printf("Index violation!\n");
return;
}
else
printf("Passed index validation: %d, as %u\n", index, index);
for(i = 0; i < index; i++)
{
if(i == 20)
{
printf("Loop overrun, undefined behavior\n");
return;
}
}
}
main()
{
int ints[] = { 1, 2, 3, 4, 5, 6, 7, 8 };
print_iterator(ints, -1);
}
If the index argument is declared as an unsigned int, it seems it will ALWAYS pass the bounds check when -1 is passed as the parameter, and produce this output:
Quote: Passed index validation: -1, as 4294967295
Loop overrun, undefined behavior
Press any key to continue . . .
If it is declared as an int (uncomment the first function line, comment out the second one) it will work properly:
Quote: Index violation!
Press any key to continue . . .
Anyone know why?
|
|
|
|
|
Member 14088880 wrote:
If the index argument is declared as an unsigned int, it seems it will ALWAYS pass the bounds check when -1 is passed as the parameter...
Of course, hence why I said the for() loop would run for a long time.
Member 14088880 wrote:
If it is declared as an int (uncomment the first function line, comment out the second one) it will work properly:
And that would be expecxted behavior.
Why do you have an aversion in using a range check on index?
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"You can easily judge the character of a man by how he treats those who can do nothing for him." - James D. Miles
|
|
|
|
|
You have nothing to indicate what the size of the array actually is. In main() you pass an array of 8 ints, but then do a later check for 20. You should be passing the array length:
void print_iterator(int integer_array[], size_t integer_array_len, size_t index) Thus the call to print_iterator would be:
print_iterator(ints, sizeof(ints) / sizeof(int), (size_t) -1); An alternative is to have a known stop character. Say -1 ends the array. You would then loop until the index or -1, whichever comes first.
|
|
|
|
|
Yes I know, the code sample I provided was only to explore / demonstrate the unsigned int behavior.
Thanks.
|
|
|
|
|
What you are ignoring is the code is invalid and would fail any software audit .. you handed that junk to me and I would sack you.
What you are ignoring if you turn the compiler warning levels to max then your code won't compile .. it will give you a fatal error. At half-arse programmer wannabe settings it probably just gives you a warning about passing a signed integer into an unsigned integer interface.
However in the real commercial world you run the compiler settings max and your code must not invoke any warning at all. Any good university when you submit your code for evaluation will have the compiler set to max you generate a warning you fail.
In vino veritas
|
|
|
|
|
leon de boer wrote: What you are ignoring is the code is invalid and would fail any software audit .. you handed that junk to me and I would sack you.
I'm writing proof/test of concept code to enhance my understanding of safe and secure programming practices. Of course it's invalid, that's the actual point. You should fire the guy who tests the security of code on your customers instead 
leon de boer wrote:
What you are ignoring if you turn the compiler warning levels to max then your code won't compile .. it will give you a fatal error. At half-arse programmer wannabe settings it probably just gives you a warning about passing a signed integer into an unsigned integer interface.
Actually I wasn't aware of how to force my IDE to use the strictest evaluation, but I know now, thanks for the tip. You may be disappointed as it notices the issue now but only generates a warning:Quote: warning C4245: 'function' : conversion from 'int' to 'unsigned int', signed/unsigned mismatch
leon de boer wrote: However in the real commercial world you run the compiler settings max and your code must not invoke any warning at all. Any good university when you submit your code for evaluation will have the compiler set to max you generate a warning you fail.
Do they also run static code analysis? Are there any freeware tools I can use? Haven't looked into it yet.
|
|
|
|
|
Well,
I hope this is a question by a student. It would prove that there is at least One student still studying the 'C' language in this world. It also gives me a dark impression of your teacher. (unless if you missed lessons, or was not paying attention)
The situation is clearly not understood by you in terms of Bits and Bytes.
Signed or Unsigned is all a matter of interpretation, and, something called 2 compliment arithmetic. One Byte of 8 bits can only have 256 possible values, 0 to 255! (Hope you know and understand that much) The question is, how do we interpret this. We can take this as an unsigned range of 0 to 255, or, as a range of -127 to + 127. In the latter case the most significant bit will represent the sign. 1 for Negative, 0 for positive. The beauty of this scheme is the Two Complement Inversion. Every Number can have it's sign changed by first inverting all bits, and then adding One. Addition and Subtraction using that scheme works correctly all the time. It is so good, the machine is not interested in whether a byte is signed or unsigned! It uses the same mechanism to do arithmetic for both!
A Byte of value 0xFF will per definition always be interpreted as 255 by the compiler if it is of unsigned type. It will also always be interpreted by the compiler as -1, if it is of signed type.
Actually, an Unsigned 32 bit Int initialised to -1, would have the value 4294967295, the same as an Unsigned Byte initialised to -1 would have the value of 255. The Compiler is friendly! If you initialise a signed byte with 255, it's value will be -1!
There is hence absolutely no point in testing an unsigned integer for being negative! It has no capacity of being negative! A half decent compiler will probably optimise the test for this condition out! An Unsigned Integer is always, and, per definition, equal or larger than 0. I hope I managed to explain in the above as to WHY!
As to your problem of access violations,
After a Brief look at your code: (unsigned int32)-1 means for the compiler: 4294967295.
So Your essentially loop says for(i=0; i<4294967295;i++){...} Make your iterator Signed! Tell me how you get on after that!
Regards
Bram van Kampen
modified 12-Jan-19 20:25pm.
|
|
|
|
|
Before I got your reply I did a little more investigating into the issue. I was looking at the bitwise representation which I noticed is 2C as you said.
I wrote this small program to test it out. Here is the output:
11111111111111111111111111111111
11111111111111111111111111111111
(int)uint_neg_1;
11111111111111111111111111111111
int_neg_1 : %d: -1, %u: 4294967295
uint_neg_1: %d: -1, %u: 4294967295
(int)uint_neg_1: %d: -1, %u: 4294967295
int_neg_1 < 0: True
uint_neg_1 < 0: False
(int)uint_neg_1 < 0: True
So it seems that the bitwise storage and representation for signed and unsigned integers is the same.
The typecast to int changed the evaluation of the expression. I'd like to see / understand the difference in the assembly instructions because I think that is probably where the paths diverge.
Quote: There is hence absolutely no point in testing an unsigned integer for being negative! It has no capacity of being negative!
That's what I believed, but based on my experiment it's not safe to rely on the "unsigned" type as an indication of being >= 0.
If someone invokes my function with "-1" and it is written without the bounds check, the result is an access violation. As a design practice my code should catch the error of the caller.
I have a bad headache, I apologize if I've made mistakes.
See this post for the use case of an unsigned int < 0 check:
Re: (C) Robust code Should an unsigned int array index be tested for "< 0" - C / C++ / MFC Discussion Boards[^]
#include <stdio.h>
#define print_binary(a) \
for(j = 31; j >= 0; j--) \
printf("%c", a & (1 << j) ? '1' : '0'); \
printf("\n\n");
void main()
{
int j;
int int_neg_1 = -1;
unsigned int uint_neg_1 = -1;
printf("int int_neg_1 = -1;\n");
print_binary(int_neg_1);
printf("unsigned int uint_neg_1 = -1;\n");
print_binary(uint_neg_1);
printf("(int)uint_neg_1;\n");
print_binary((int)uint_neg_1);
printf("int_neg_1 : %%d: %d, %%u: %u\n", int_neg_1, int_neg_1);
printf("uint_neg_1: %%d: %d, %%u: %u\n", uint_neg_1, uint_neg_1);
printf("(int)uint_neg_1: %%d: %d, %%u: %u\n\n", (int)uint_neg_1, (int)uint_neg_1);
printf("int_neg_1\t< 0: %s\n", int_neg_1 < 0 ? "True" : "False");
printf("uint_neg_1\t< 0: %s\n\n", uint_neg_1 < 0 ? "True" : "False");
printf("(int)uint_neg_1\t< 0: %s\n\n", (int)uint_neg_1 < 0 ? "True" : "False");
}
|
|
|
|
|
Member 14088880 wrote:
And that is correct. An unsigned number can only have positive values in the range 0 to infinity. The main point here is that you must follow the rules of mathematics and logic. If you declare a numeric variable as unsigned then that is how it will be treated, and it will never be considered to have a negative value (even if it does have the sign bit set). The compiler will add code to manipulate the value in such a way that any 'negativeness' is always ignored.
|
|
|
|
|
It seems like in this signed/unsigned case typecasting allows you to change the way the variable is evaluated in expressions. I think that is typical of typecasting in general, it changes interpretation.
On the other hand I think if you typecast to a data type with a different size it does not just affect the expression but also the raw value.
Please see my newest reply to the main topic.
Thanks
|
|
|
|
|
Exactly so. When you use a cast on any variable you are effectively telling the compiler to ignore the rules, as you know what you are doing, even if it is against the rules.
|
|
|
|
|
I wrote a small demonstration of the issue. Peter and Paul are writing ticketing system software and Peter makes a mistake swapping two variables and creating a negative number which is passed as an unsigned integer to Paul.
Paul makes a mistake thinking an unsigned int will not cause an out of (lower) bounds array access if it passes the test "index >= 0".
Peter makes a mistake, Paul fails to catch it, the memory is potentially corrupted and undefined behavior results. That's the PRINCIPLE the code is meant to demonstrate, which is the only purpose of this code.
As an alternative I put in a boundary check based on the address the index would have us access and the addresses of the first and last members of the array. I think that's a better approach. It should match the way the array is accessed exactly and it should be completely independent of variable size, type, typecasting, and bitwise representation.
As an aside, VS on the maximum warning level does not warn for the ints passed to the function that was declared with unsigned ints. Also, the compiler can't foresee that a negative value will be passed to the function because the sign of that variable is determined at run time.
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int get_available_seat_count(unsigned int *seating_counts, unsigned int row_index, unsigned int num_rows, unsigned int seats_per_row)
{
if(row_index < 0)
{
printf("row_index < 0: TRUE\n");
return -1;
}
else
printf("row_index < 0: FALSE\n");
if(&seating_counts[row_index] < &seating_counts[0])
printf("Array minimum boundary violation, index is %d\n", row_index);
if(&seating_counts[row_index] > &seating_counts[num_rows-1])
printf("Array maximum boundary violation, index is %d\n", row_index);
return seats_per_row - seating_counts[row_index];
}
void main()
{
int num_rows = 12;
int seats_per_row = 40;
int n_available_seats;
int row_index;
unsigned int seating_counts[12];
int i;
srand((int)time(NULL));
for(i = 0; i < num_rows; i++)
seating_counts[i] = (unsigned int)rand() % seats_per_row;
printf("Customer: are there any seats available in the third row from the stage?\n\n");
row_index = 3;
n_available_seats = get_available_seat_count(seating_counts, row_index - num_rows, num_rows, seats_per_row);
printf("\nTeller: there are %d seats available in that row\n", n_available_seats);
}
Quote: Customer: are there any seats available in the third row from the stage?
row_index < 0: FALSE
Array minimum boundary violation, index is -9
Teller: there are 858993500 seats available in that row
|
|
|
|
|
But row_index is declared as unsigned int so, as I have already explained, it can never be less than zero.
|
|
|
|
|
I haven't read all of the responses in detail, so excuse me if some of the below has already be mentioned.
1. If the function argument is defined as unsigned, a signed int will be silently transformed into an unsigned leading to potentially inappropriate behavior from the viewpoint of the caller (already mentioned). The main issue here is the silent transformation that may at best be indicated as a warning at compile time. For that reason, it might be worth considering to change the argument type to signed. Some of the big guys in C++ programming think that the use of unsigned is often not worth the hassle and may convey a false sense of security. Point in case: if you have a variable of type array index, it's true that a negative value doesn't make sense. But defining it unsigned still doesn't make it range-safe as you still need to check the upper bound. Being unsigned safes you one check, but introduces the new problem of silent signed to unsigned conversions. In the end you gain nothing.
2. The test <0 always returns true on an unsigned variable. Therefore there is no point to make it. If you want to make a two-sided bounds check on an unsigned, checking just the upper bound actually treats both, because negative values will be converted to very large positive values!
3. regarding #3: don't worry about possible future extensions! Make your code look reasonable and meaningful from the point of view of the requirements that you have now! Chances are, that any future requirement will look different than you're anticipating. Furthermore, if anything problematic of a scope you're describing here is going to happen, a lot more than your function will be affected, and it's quite possible someone will come up with some generic workaround that can cope with this problem; however, if you make your code try to anticipate that change, the generic code may not work on it, because it doesn't look as expected! Better just design your code in the most reasonable way now!
4. You can avoid a lot of these problems if you simply use std::array
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
