GCC bug or am I doing something wrong? - maemo.org

pichlo | # 1 | 2014-06-11, 22:26 | Report

I am fiddling with Qt and, as always, writing and compiling the code on the phone itself. All was hunky-dory for many edit-and-compile iterations until I suddenly out of the blue started receiving this error:

Code:

{standard input}: Assembler messages:
{standard input}:766: Error: bad instruction `orn r3,r3,#127'
make: *** [waveform.o] Error 1

This is the code that triggered all the fuss:

Code:

void WaveForm::on_comboFreqPeriod_currentIndexChanged (int index)
{
  ui.comboFreqPeriodUnit->clear();
  switch (index)
  {
  case FREQUENCY:
    ui.comboFreqPeriodUnit->addItem("Hz");
    ui.comboFreqPeriodUnit->addItem("kHz");
    break;
  case PERIOD:
    ui.comboFreqPeriodUnit->addItem(micro + "s");
    ui.comboFreqPeriodUnit->addItem("ms");
    ui.comboFreqPeriodUnit->addItem("s");
    break;
  default: // impossible but let's keep gcc happy
    break;
  }
  ui.comboFreqPeriodUnit->setCurrentIndex(1);
}

Removing the blue bit makes it behave again. I even suspected some hidden non-ASCII sneaking into the code somewhere, so I deleted the offending lines completely and wrote them again from scratch. Same results.

Now here comes the interesting part. Various vays of editing the code got rid of the error:

Add a declaration of something involved at the beginning of the function. It does not need to be used, as long as the con/destructor is being called. An int was not enogh, but e.g. an QStringList did the trick.
Split the string literal in one of the blue lines to something like QString("k") + "Hz".
Change switch(index) to switch(index+1), with obvious changes to the case labels.

What did not work:

Change the string literals.
Add an another addItem line with a string literal argument.
Remove one of the blue lines (removing both worked though).

All this points me in the direction of code size. In all cases that fixed the problem, a significant amount of additional code was generated or removed. My theory is that the compiler generates the code in multiples of X bytes and adds padding as necessary, and that the orn r3,r3,#127 instruction is somehow involved in that. Adding or removing enough code changes the padding and avoids using the instruction.

Does anyone have a better explanation or, better still, a solution? I am using gcc 4.6.1 from extras-devel, if that makes any difference, and am using the CFLAGS trick suggested here to build Qt on the phone. Perhaps another flag is required?

The Following 2 Users Say Thank You to pichlo For This Useful Post:
backcover_press_service, juiceme

Copernicus | # 2 | 2014-06-12, 01:05 | Report

Originally Posted by pichlo View Post

Add a declaration of something involved at the beginning of the function. It does not need to be used, as long as the con/destructor is being called. An int was not enogh, but e.g. an QStringList did the trick.
Split the string literal in one of the blue lines to something like QString("k") + "Hz".
Change switch(index) to switch(index+1), with obvious changes to the case labels.

Hmm, I wonder if there's some magic involved with the indexing here, especially considering the last item on that list. C compilers can try to do some pretty crazy optimization on switch statements, so yeah, I can believe that the compiler might make a mistake somewhere...

Can you provide some info as to how FREQUENCY and PERIOD are defined? Are they #defines, enums, variables? What are their values? (GCC might be utilizing the raw values in its jump tables, which is why their values could be interesting.)

I haven't personally done any assembler or compiler stuff in a very, very long time, so take anything I say with a grain of salt.

But, I just did a quick google search to refresh my memory on the subject; here's an interesting description of why to be wary of the switch statement:

http://embeddedgurus.com/stack-overf...ch-statements/

On another topic, if there are only two possible values, it might be easier to just skip the switch statement altogether.

An if/else statement should be just as efficient, if not more so:

Code:

if (index == FREQUENCY)
{
// Add the Hz items
}
else
{
// Add the Seconds items
}

Last edited by Copernicus; 2014-06-12 at 01:20.

The Following User Says Thank You to Copernicus For This Useful Post:
juiceme

shawnjefferson | # 3 | 2014-06-12, 04:48 | Report

I don't believe any compiler would add padding to a program. Sounds like perhaps a bug with the optimizer maybe, and changing the code enough causes the optimizer to do something differently that doesn't provide the error. Do you get the same error compiling with different levels of optimization?

pichlo | # 4 | 2014-06-12, 07:35 | Report

Hmm, haven't tried varying the optimization level, a good point. But I have seen an assembly output of various C and C++ programs (not this one yet though) and there often was a few bytes of padding with NOPs before or after the final RET from the subroutine.

Copernicus, FREQUENCY and PERIOD are enums with values 0 and 1, respectively. You are right, there are only those two values so I could have used an if instead of a switch but I think the latter communicates the intention more clearly.

FWIW. the code has been completely refactored since last night and no longer exhibits the problem.

The Following 2 Users Say Thank You to pichlo For This Useful Post:
backcover_press_service, Copernicus

Halftux | # 5 | 2014-06-12, 08:52 | Report

Originally Posted by Copernicus View Post

here's an interesting description of why to be wary of the switch statement

It is also possible to use enum instead of switch.

Maybe this is interesting.
http://schneide.wordpress.com/2010/1...itch-use-enum/

pali | # 6 | 2014-06-12, 11:20 | Report

Try -march=armv7-a or -march=native

Copernicus | # 7 | 2014-06-12, 12:06 | Report

Originally Posted by Halftux View Post

It is also possible to use enum instead of switch.

Maybe this is interesting.
http://schneide.wordpress.com/2010/1...itch-use-enum/

While this is a cute way to use an enum, I don't think it'd be something I'd do.

IMHO, the best value of an enum is to make clear which of a variety of states your code is in. (This is Pichlo's quite valid criticism of my if/then suggestion, as I skipped the use of one of his two enum values.) If you hide the code inside the enum declaration itself, you're basically moving from declaring a set of states to declaring a set of function pointers, which is a very different beast.

Besides, we've got C++ here.

I think it'd be better to go all the way and create a set of sibling classes. Something like this:

Code:

class indexType
{
public:
  virtual void addItems() = 0;
};

class frequency: public indexType
{
public:
  void addItems(); // Add the Hz items
};

class period: public indexType
{
public:
  void addItems();  // Add the Seconds items
};

The Following 2 Users Say Thank You to Copernicus For This Useful Post:
backcover_press_service, pichlo

pichlo | # 8 | 2014-06-12, 14:10 | Report

@Copernicus,
Thanks, that is a nice way to do it but I have already refactored it even further. As I have quite a bunch of combo boxes that change contents each time one of the other combos change, I thought it made sense to write a single function:

Code:

static void refillComboBox (QComboBox *combo,
                            const QString items[],
                            int number_of_items,
                            int selected_item);

and call it like this:

Code:

#define ITEMS(x)  sizeof x / sizeof x[0]

static const QString foo[] = { "Hz", "kHz" };
refillComboBox(ui.comboBlaBlaBla, foo, ITEMS(foo), 0 };

static const QString bar[] = { micro + "s", "ms", "s" };
refillComboBox(ui.comboBlaBlaBla, bar, ITEMS(bar), 3 };

...etc.

@Halftux,
I happen to agree with Copernicus. A cute way of (ab)using an enum but I would not go there.

@pali,
Of course I am using -march=armv7-a. That is the CFAGS trick mentioned in the OP. Without it, nothing works.

The Following 2 Users Say Thank You to pichlo For This Useful Post:
backcover_press_service, Copernicus