unerklärliche Code-Optimierung

[Ralf (Gast)]

Mahlzeit!

Ich habe mich gestern entschieden, mal doch mit C anzufangen und bin 
dabei ein Assemblerprojekt von mir zu übertragen. Jetzt habe ich hier 
eine Funktion, die ich gerade mal durch den Simulator schicke:

void Steuerung(uint8_t *DatenNeu)
{
 if (DatenNeu)
 {
  uint8_t BefehlPos=~0;
  uint8_t i;
  for (i=0; i < W_AnzBefehl; i++)
   if (0==strncmp((char*)&DatenNeu[3],(char*)Befehle[i],W_StatusLen))
    BefehlPos=i;
  i=0;
  while (BefehlPos == ~0)
  {
   if (Befehle[i]==0)
   {
    strncpy((char*)&DatenNeu[3],(char*)Befehle[i],W_StatusLen);
    BefehlPos=i;
   }
   else
    i++;
  }
  BefehlPos++;
  if (BefehlPos > (W_AnzBefehl >> 1))
   BefehlNr=(BefehlNr & 0x0F) | BefehlPos;
  else
   BefehlNr=(BefehlNr & 0xF0) | BefehlPos;
 }

// spezielle Befehle, je nach Platine:


};

Der Compiler haut mir die 'while-Schleife' komplett raus (auch ohne 
Optimierung!). Unverschämtheit ;-)
Wie bekommt man das weg?

[TestX .. (xaos)]

benutz als operator ungleich..

[MaWin (Gast)]

In deiner while Schleife wird geguckt, ob ein Befehle[i] == 0 
(NULL-Pointer) ist.

Wenn keiner der Befehle[i] NULL ist, kann es auch in Zukunft keiner
werden.

Also würde in der while-Schleife nur i++ aufgeführt werden.

Aber i interessiert hinterher keinen mehr.

Da vermutlich alle Befehle[i] ungleich NULL sind,
sondern auf "Worte" zeigen (sonst würde das strncmp
auch mit illegalem Zugriff auf NULL-Pointer scheitern)
kann also die Schleife komplett entfernt werden.

Sei froh, daß dich der Compiler auf dermassen bescheuerten
Code hinweist.

[Yalu X. (yalu) (Moderator)]

Mit welchem Compiler arbeitest du? Falls mit dem GCC:

Wenn du die Option -Wall aktivierst, erhältst du für die Zeile mit dem
while eine Warnung, die dich auf deinen Fehler hinweist:

  warning: comparison is always false due to limited range of data type

Und warum?

~0 ist gleich -1, da 0 eine vorzeichenbehaftete Integerzahl ist.

BefehlPos kann aber als uint8_t-Variable nie negativ werden, also
schlägt der Vergleich BefehlPos==~0 immer fehl. Das weiß auch der
Compiler.

Du kannst jetzt entweder den Datentyp von BefehlPos in int8_t ändern,
den Wert ~0 in ein uin8_t casten oder ~0 gleich durch 0xff oder 255
ersetzen. Welches davon die beste Lösung ist, hängt davon ab, was die
tiefere Bedeutung von BefehlPos ist.

Edit: Da liegt aber noch mehr im Argen:

   if (Befehle[i]==0)
   {
    strncpy((char*)&DatenNeu[3],(char*)Befehle[i],W_StatusLen);

Ist Befehle[i] tatsächlich 0, über gibst du der strncpy-Funktion einen
Nullzeiger, was sicher nicht deine Absicht ist.

[Ralf (Gast)]

MaWin schrieb:
> Sei froh, daß dich der Compiler auf dermassen bescheuerten
> Code hinweist.

Ich nehm's mal nicht persönlich.
Einen Fehler habe ich schon korrigiert:

if (Befehle[i][0]==0)

'Befehle' ist ein Array von Zeichenketten in dem am Anfang nichts steht.
Es sollen Daten an eine freie Position kopiert werden. Diese Position 
merkt sich dann 'BefehlPos'. Wenn ein Element schon belegt ist, dann 
wird 'i' inkrementiert.

[Ralf (Gast)]

Yalu X. schrieb:
> Edit: Da liegt aber noch mehr im Argen:

hatte ich zwischenzeitlich schon bemerkt, wurde aber beim Schreiben 
abgelenkt. Trotzdem: Danke!

[Läubi .. (laeubi)]

Ralf schrieb:
> Wie bekommt man das weg?

Zu den Fehlern wurde ja schon etwas gesagt. Allgemein: Gewöhn' dir 
besser an generell die {} klammern um alle Ausdrücke zu setzen, so 
verhinderst du gleich merkwürdige Effekte, besonders den Stil innerhalb 
eines Blocks zu wechseln ist extrem verwirrend.

Wenn auch in Assembler galt, weniger Befehle => kürzeres/schnelleres 
Programm, sollte man auf solche "Optimierungen" in C verzichten, du 
haust dir da nur Fehler rein die nicht sein müssen.

Auch das "Wiederverwenden" von i, einmal als Schleifenvariable und 
einmal als Zähler für die While Schleife ist einerseits unötig (der 
Compiler optimiert das schon von selbst) und wird dir früher oder später 
auf die Füße fallen, Variablen sollten immer einen so kleinen 
Gültigkeitsbereich wie möglich haben.

[Ralf (Gast)]

Yalu X. schrieb:
> Wenn du die Option -Wall aktivierst, erhältst du für die Zeile mit dem
> while eine Warnung, die dich auf deinen Fehler hinweist:
>
>   warning: comparison is always false due to limited range of data type
>
> Und warum?
>
> ~0 ist gleich -1, da 0 eine vorzeichenbehaftete Integerzahl ist.

avr-gcc  -mmcu=attiny2313 -Wall -gdwarf-2 -std=gnu99 -O0 -funsigned-char -funsigned-bitfields -fpack-struct -fshort-enums -MD -MP -MT Steuerung.o -MF dep/Steuerung.o.d  -c  ../Steuerung.c

Das wird beim Build angezeigt, da ist die Option doch dabei?

Ich hätte gedacht: ~0 -> mit Vorzeichen = -1; ohne Vorzeichen = 255 ??
Jedenfalls: mit 0xFF funktioniert's.

[MaWin (Gast)]

void Steuerung(char *Befehl) aufgerufen mit DatenNeu+3
{
  uint8_t BefehlPos ;
  for (BefehlPos = 0;BefehlPos < W_AnzBefehl; BefehlPos ++)
  |
   if(Befehle[BefehlPos][0]=='\0')
   {
     strncpy(Befehle[BefehlPos],Befehl,W_StatusLen);
     break;
   }
   if (strncmp(Befehl,Befehle[BefehlPos],W_StatusLen)==0)
   {
     break;
   }
  }
  if(BefehlPos<W_AnzBefehl)
  {
    // Befehl gefunden bzw. eingetragen, sehr mysteriöser code
    if (BefehlPos > (W_AnzBefehl >> 1))
     BefehlNr=(BefehlNr & 0x0F) | BefehlPos;
    else
     BefehlNr=(BefehlNr & 0xF0) | BefehlPos;
  }
  sonst Befehle voll
}

[Ralf (Gast)]

noch mal zum '~0'-Verständnis:
Müsste da nicht auch bei z.B. ...

REG|=~(1 << Bit); // irgend ein Bit löschen, z.B. in einem Timer- oder UART-Register

... Mist rauskommen? Der Wert wird doch auch negativ.

[Patrick (Gast)]

~ invertiert alle Bits, aber | verORt die ganze Chose.

Mit anderen Worten: Mit dem Befehl wird nix gelöscht, sondern alle Bits 
in REG bis auf das (Bit)-ste Bit gesetzt.

In Worten: REG wird gesetzt auf (REG ODER (alle Bits invertiert von 
(Dezimalzahl 1, um (Bit) Stellen nach links geschoben))).

[Ralf (Gast)]

Patrick schrieb:
> ~ invertiert alle Bits, aber | verORt die ganze Chose.
>
> Mit anderen Worten: Mit dem Befehl wird nix gelöscht, sondern alle Bits
> in REG bis auf das (Bit)-ste Bit gesetzt.
>
> In Worten: REG wird gesetzt auf (REG ODER (alle Bits invertiert von
> (Dezimalzahl 1, um (Bit) Stellen nach links geschoben))).

Ja Mist, geträumt. :-(
So natürlich:

REG&=~(1 << Bit); // irgend ein Bit löschen, z.B. in einem Timer- oder UART-Register

[Yalu X. (yalu) (Moderator)]

Ralf schrieb:
> Müsste da nicht auch bei z.B. ...
> REG|=~(1 << Bit); // irgend ein Bit löschen, z.B. in einem Timer- oder 
UART-Register
> ... Mist rauskommen? Der Wert wird doch auch negativ.

Stimmt, der Wert wird auch hier negativ. Bei der Zuweisung des
Ergebnisses an REG werden aber die oberen 8 Bits des Ergebnisses
abgeschnitten, und die unteren 8 Bits haben immer den gleichen Wert,
egal ob die Rechnung in signed oder unsigned durchgeführt wurde.

Bei folgendem Vergleich, der deinem von oben entspricht, findet aber
keine Zuweisung, sondern nur eine Rechenoperation (in diesem Fall ein
Vergleich) statt

  u8var == -1

Für Rechenoperationen mit Operanden ungleichen Datentyps wird der
Wert des Operanden mit dem kleineren Datentyp auf den Typ des größeren
Operanden erweitert, in diesem Fall also u8var auf den Typ int. Hat
u8var den Wert 255 (0xff) ist der Wert auch nach der Erweiterung 255
(0x00ff). Es wird also 255 mit -1 verglichen.

Anders sieht es aus, wenn die Variable vorzeichenbehaftet ist: Hat in

  s8var == -1

s8var ebenfalls den Wert 0xff, wird dieser nicht als 255, sondern als -1
interpretiert, was auf int erweitert ebenfalls -1 (0xffff) ergibt. Dann
würde der Verlgeich wie erwartet funktionieren.

In C sind Berechnungen mit unterschiedlichen Operandentypen oft nicht
leicht durchschaubar. Wenn man sich mit mit den Promotionsregeln nicht
ganz sicher ist, sollte man das Mischen unterschiedlicher Datentypen
möglichst vermeiden, indem man die Variablentypen geeignet wählt und bei
Bedarf explizite Casts verwendet.

[Ralf (Gast)]

@Yalu X.
Aaah. Konstanten werden also immer erstmal als int behandelt? Außer sie 
werden mit L oder ULL oder sowas erweitert?

[Ralf (Gast)]

while (BefehlPos == (uint8_t)~0)

müsste dann sozusagen funktionieren?

[Yalu X. (yalu) (Moderator)]

> Aaah. Konstanten werden also immer erstmal als int behandelt? Außer sie
> werden mit L oder ULL oder sowas erweitert?

> while (BefehlPos == (uint8_t)~0)
>
> müsste dann sozusagen funktionieren?

Zweimal ja.

[Ralf (Gast)]

Yalu X. schrieb:
> Edit: Da liegt aber noch mehr im Argen:
>    if (Befehle[i]==0)
>    {
>     strncpy((char*)&DatenNeu[3],(char*)Befehle[i],W_StatusLen);

und: Quelle mit Ziel verwechselt.

---
Falls noch Zeit ist:
Die Idee hinter der Funktion:
- Übernahme eines Zeigers auf ein Datenpaket (vom UART mit Header, 
deshalb (char*)&DatenNeu[3])
- Nachsehen, ob dieser 'Befehl' schon mal gesendet wurde, dann Position 
zurück
- wenn nicht, hinten dranhängen, Position merken
- Position global zugänglich machen in 'BefehlNr' (muss für die erste 
Hälfte im unteren Nibble, für die zweite Hälfte im oberen Nibble stehen. 
Soll einfach so sein!)

Ist die Umsetzung dazu wirklich so schlimm?

[Karl H. (kbuchegg)]

Ralf schrieb:

> Ist die Umsetzung dazu wirklich so schlimm?
Ja, schon.



Zeig mal deine Datentypen.

> Die Idee hinter der Funktion:
> - Übernahme eines Zeigers auf ein Datenpaket (vom UART mit Header,
> deshalb (char*)&DatenNeu[3])

Die Funktion sollte bereits den Pointer auf den Befel erhalten. Dass der 
Befehl am 3. Byte des Datenpakets beginnt, ist etwas was diese Funktion 
nicht berühren sollte. Das ist 'Wissen', welches nicht in diese Funktion 
gehört, sondern vom Aufrufer der Funktion erledigt werden soll.

Diese Funktion arbeitet mit Strings. Daher sollte der Pointer, den die 
Funktion bekommt auch bereits ein char Pointer sein und kein uint8_t 
Pointer. Du castest mir da in der Funktion etwas zuviel rum, ohne dabei 
einen wirklich guten Grund zu haben (ausser, dass du die Dinge 
möglicherweise ungeschickt aufgebaut hast. Wozu musst du Befehle[i] auf 
einen char* casten? Mach das doch gleich als char Array.
Du nimmst casten auf eine etwas zu leichte Schulter. Bei jedem, aber 
auch wirklich bei absolut jedem Cast, solltest du dir überlegen, warum 
da ein Cast sein muss und ob es nicht eine Möglichkeit gibt, den Cast 
loszuwerden. Cast sind Waffen! Sie sind nur allzuoft die Anweisung an 
den Compiler: "Hör mal zu! Ich weiß, dass die Datentypen nicht passen. 
Aber halte jetzt einfach mal die Schnauze und tu so, als ob sie 
tatsächlich passen würden. Ich übernehme die Verantwortung dafür!".
Oft geht das auch gut. Aber noch öfter geht das schief, weil der 
Compiler tatsächlich recht hatte und das ganze Zeugs wirklich nicht 
passt. Die eifrigen Caster überstimmen dann den Compiler mit einem Cast 
und suchen dann stundenlang den Fehler.
Also: Casten nur dann, wenn es nicht anders geht!


> - Nachsehen, ob dieser 'Befehl' schon mal gesendet wurde, dann Position
> zurück
> - wenn nicht, hinten dranhängen, Position merken
> - Position global zugänglich machen in 'BefehlNr' (muss für die erste

Das sind 2 Themenkreise, die man wunderbar voneinander trennen kann und 
wo sich jeder der beiden Themenkreise eine eigene Funktion verdient 
hätte. Warum tust du das nicht? Zumal man eine Find Funktion sicherlich 
auch anderswo gut gebrauchen kann.

//////////////////////
//
uint8_t Steuerung( const char* String )
{
  // Sicherung
  if( String == NULL )
    return 0xFF;

  // wenn es den Befehl noch nicht gab, dann lege ihn an
  uint8_t BefehlPos = FindBefehl( String );
  if( BefehlPos == 0xFF )
    BefehlPos = InsertBefehl( String );

  return BefehlPos;
}

//////////////////////
//
uint8_t FindBefehl( const char* String )
{
  uint8_t i;

  for( i = 0; i < W_AnzBefehl; ++i ) {
    if( strcmp( String, Befehle[i] ) == 0 )
      return i;
  }

  return 0xFF;  // nicht gefunden
}

//////////////////////
//
uint8_t InsertBefehl( const char* String )
{
  uint8_t i;

  for( i = 0; i < W_AnzBefehl; ++i ) {
    if( *Befehle[i] == '\0' ) {
      strncpy( Befehle[i], String, W_StatusLen );
      Befehle[W_StatusLen-1] = '\0';   // Sicherung fuer strncpy

      // deine Spezialbehandlung für BefehlNr, aus deiner Beschreibung
      // werde ich nicht schlau. Sie klingt unlogisch und ich denke du
      // verwechselst da etwas.
      // musst du selber einsetzen

      return i;
    }
  }

  return 0xFF;  // kein Platz mehr
}

> wenn nicht, hinten dranhängen

wirklich hinten drann?
Dein Code hängt nicht hinten drann, sondern sucht im Befehls Array eine 
freie Stelle. Hinten drann geht einfacher, wenn man sich ganz einfach 
die Anzahl der gespeicherten befehle merkt. Das beschleunigt dann auch 
die Find Funktion enorm.



und das die Sache mit ~0 keine so gute Idee war, hast du ja mitlerweile 
selber schon rausgefunden.

[Ralf (Gast)]

Abschlussbericht
@Yalu X.
Das war der entscheidende Hinweis zur Ergreifung des Täters.
Und ich finde das so gut mit ~0 :-(. So im wörtlichen Sinne: das 
Gegenteil von null. Wie true und false, nur zusätzlich noch mit Zahlen. 
Auf dem PC gab's da nie Probleme. Werde mal prinzipiell auf 0xFF 
umstellen.

@Karl Heinz
100% Übereinstimmung:
 - Zeigerübergabe
 - cast

Die Schlamperei war sozusagen geistig schon geändert. Ich suchte ein 
schnelles Erfolgserlebnis. Wollte erstmal Fehlermeldungen + Warnungen 
wegbekommen. Und die lustigen Kommentare :-( im Simulator haben mich 
früher von C beim µC-Programmieren immer wieder weggeholt. Z.B. 
'location not valid'. Das hat vorgestern eine Weile gedauert, bis ich 
gemerkt habe, dass der Simulator nur heult, weil der Compiler (auch ohne 
Optimierung) die Variable in einem mehrfach benutzen Register versteckt 
hat.

50% Übereinstimung:
- mehrere (hier natürlich nur eins am Anfang) returns aus einer 
Funktion.

Spart Klammerebenen. Sieht eventuell etwas übersichtlicher aus. Was ist 
aber, wenn später vor dem (jedem!) return noch ein paar Anweisungen 
eingefügt werden sollen? Alle returns raussuchen, alles gut in Klammern 
verpacken, nichts vergessen. Ich schreib's am Ende nur dazu.
Muss ich noch drüber nachdenken.

Plan:
Selbstverständlich Funktionen für Finden und Einfügen. Und eventuell 
auch für strncpy und strncmp. Hab’ schon mal gebastelt. Da Länge < 256 
reicht, spart das noch mal Code (strncpy: 15 ASM-Befehle/ Eigene: 9 
ASM-Befehle). So als 'Fingerübung' und 'Kleinvieh macht auch Mist'.

@all
Vielen Dank

[Läubi .. (laeubi)]

Ralf schrieb:
> Was ist
> aber, wenn
Wenn das Wörtchen Wenn nicht wär' ;)

Wenn das mal nötig sein sollte kann man das immer noch ändern, hier 
führt es auf jedenfall dazu, das ein "anderer" nicht erst alle if else 
cas switch selcet default wasweisich durchhecheln muss um zu sehen das 
Die Funktion im "Fehlerfall" 0xFF zurück gibt.

Ralf schrieb:
> So im wörtlichen Sinne: das
> Gegenteil von null
Nur hat 0 kein Gegenteil (außer im System zur Basis 2)... Und dein 
"Gegenteil" ist halt abhängig von der Wortbreite was nicht so schön ist 
wie du schon feststellen musstest...

Wenn man also auf 0xFF vergleichen will dann schreibt man das auch 
hin... den 0xFF ist nicht ~0 (außer in Ausnahmefällen) und der Leser 
muss wieder denken: Wieso steht das da, hat das etwa eine Bedeutung, was 
hat sich der Autor gedacht...