Compiler/MSP430F5338: Adding offsets larger than 32k to pointers in restricted data model.

Unfortunately, I cannot reproduce the problem, so I am not sure what is going on.  Please submit a test case.

Preprocess the source file which contains the problem source statement.  Indicate where it is by adding the comment /* PROBLEM HERE */  . Attach that file to your next post.  Also show all the build options exactly as the compiler sees them.

Thanks and regards,

-George

Please ignore that request for a test case.  I can reproduce it.

But I still cannot explain why the shifts are needed.  I will consult with some experts on that.  I likely will not hear anything back until after the holidays.

Thanks and regards,

-George

George Mock said:

I can reproduce it.

[There is no SDOWP entry in the referenced thread]

Chester Gillon said:

Out of curiosity, is this the same problem as reported in MSP430 compiler v15.12.2.LTS array index calculated incorrectly for an unsigned index >= 0x8000 ?

[There is no SDOWP entry in the referenced thread]

It's definitely related, as you can see in the assembly the left and right shift before being added to the pointer.

I'm hoping that the "sooner" from June 2016 has already passed and all I need to do is upgrade compilers.

As to the shifting: the right and left shift would be appropriate if the number being added was a signed int as the shifting would sign extend it so you'd have a proper negative 20 bit number to add to your pointer. However, when you're trying to add unsigned ints or unsigned longs, this behavior creates problems with numbers greater than 0x7FFF.

I think this is related to the restricted data model assuming all non const pointers point to addresses in RAM.  However, that assumption was supposed to allow the compiler to produce smaller code, and adding in a mov and two shifts when a single add would do the same thing doesn't seem like code reduction to me.

Frederick Doering said:

I'm hoping that the "sooner" from June 2016 has already passed and all I need to do is upgrade compilers.

[I don't currently have a board to test the code, but the generated incorrect assembler was still the same as in the referenced thread].

Chester Gillon said:

Out of curiosity, is this the same problem as reported in MSP430 compiler v15.12.2.LTS array index calculated incorrectly for an unsigned index >= 0x8000 ?

I am not certain, but that is probably the same issue.  I can tell you that the ID for the issue in that older thread is CODEGEN-1412 .  Unfortunately, it has not been addressed.  As I said in the other thread, that issue is filled with compiler team jargon.  I hope it is OK that I don't expose it externally.

Thanks and regards,

-George

George Mock said:

I can tell you that the ID for the issue in that older thread is CODEGEN-1412 .  Unfortunately, it has not been addressed.

George Mock said:

I hope it is OK that I don't expose it externally.

However, I note that http://software-dl.ti.com/codegen/esd/cgt_public_sw/MSP430/16.9.1.LTS/Open_defects.html doesn't list CODEGEN-1412 as being a known open defect. Is that because the list of open defects only contains those defects which are exposed externally in the bug tracking system?

CODEGEN-1412 has now been resolved but is a separate issue than the one described in this thread. I have opened CODEGEN-1995 to track this issue.

This standalone download page for the MSP430 compiler includes a link for all Open Defects against compiler version 17.3.0.STS.  CODEGEN-1995 is among those issues.  There is a link to the SDOWP entry for the issue.  Since it is in progress, it doesn't show much.

Thanks and regards,

-George

This fix is still pending because we are still analyzing its performance implications.

The fix to this bug is quite simple.  However it introduces performance degradations in several regression tests and therefore has required more analysis prior to implementation.

Point taken.  But that is not the only consideration.

This problem has been in the compiler for a very long time.  So we know, by direct experiment, that it doesn't affect many users.  If we quickly make a change that fixes this problem, but reduces performance, that is likely to negatively affect lots of users.  In fact, nearly all of them.

So, we are trying to find a balance among these concerns.

Thanks and regards,

-George

Yes the bug has been there a long time, and the fact that it hasn't been reported before probably means that most people don't run into the bug. This is likely because the vast majority of people don't add offsets greater than 0x7FFF.

Now if they aren't going to ever offset greater than 0x7FFF they should not be using a long (unsigned or otherwise) to represent the offset. If they are using an unsigned int, they should see a performance GAIN from this bug fix as the assembly should have 3 fewer instructions and use one less register. If they are using a signed int, the code should remain the same.

If they are using a long (unsigned or otherwise) to represent their offset, they are probably doing that because it's possible that the offset is greater than can be represented in an int. This can and did result in latent bugs in supposedly tested and working code; it works for small offsets and only breaks for large ones. This is way worse than slower performance, especially as the only negative performance impact should be when people are using a long to represent an offset. If they are using a long unnecessarily, performance obviously isn't their top priority and the avoidable* degradation in performance shouldn't matter too much. If they are using a long necessarily, then they have a latent bug in their code that may be causing problems that people just haven't figured out the source. When I first found this bug, all of my colleagues told me I was wrong, and it couldn't be the compiler; the bug must be in my code. I'll bet there are quite a few other people who just found a work around never understanding the source of the bug.

I don't see an avoidable performance degradation as a barrier to fixing latent bugs in people's code. Especially when the fix also should create a performance boost for anyone using unsigned int offsets.

*anyone who wants to avoid the performance degradation can just use ints instead of longs to represent the offset.

Thanks Frederick for precisely expressing my thoughts, I'm not familiar enough (any more) with these considerations to write them down so clearly.

And yes, this clearly leads to latent bugs, I have a couple of hundreds devices out there in the wild I have to update within 200 days, that's the time they need to hit the bug.