So (16 bit variable) xor (8 bit variable) is illegal in C? Did not expect that...
Of course not. What you do not know is how the compiler is handling the calculation. If you think you know how it should be done, type the variables explicitly, as did Whandall.
People come here nearly every day wondering why the following line "gives the wrong answer":
int x = 72*1000;
Huh. I like impossible problems!
This looks like a bug. Perhaps in the C compiler itself...
Your simplified example compiles to:
// ADSC is cleared when the conversion finishes
while (bit_is_set(ADCSRA, ADSC));
364: 80 91 7a 00 lds r24, 0x007A ; 0x80007a <__TEXT_REGION_LENGTH__+0x7e007a>
368: 86 fd sbrc r24, 6
36a: fc cf rjmp .-8 ; 0x364 <loop+0x38> *** Wait for conversion ***
// we have to read ADCL first; doing so locks both ADCL
// and ADCH until ADCH is read. reading ADCL second would
// cause the results of each conversion to be discarded,
// as ADCL and ADCH would be locked when it completed.
low = ADCL;
36c: 80 91 78 00 lds r24, 0x0078 ; 0x800078 <__TEXT_REGION_LENGTH__+0x7e0078>
return write(c);
}
370: 2a e2 ldi r18, 0x2A ; 42 *** There's 41 ****
372: 28 27 eor r18, r24 ; *** There's the XOR ***
374: 30 e0 ldi r19, 0x00 ; 0
size_t Print::print(unsigned char b, int base)
;;; *** Prints the value ****
You'll note that while there is a big comment about when to read ADCH (the high byte of the result), there is never actually any access to ADCH!!!
The datasheet says:
When ADCL is read, the ADC Data Register is not updated until ADCH is read.
So your original description is exactly right - it's reading the same value from the ADC, over and over again (at least potentially. I'm not sure how the other code in analogRead() comes into play.)
It looks like the optimizer is deciding that since your code doesn't use the high byte (your bug), it isn't reading it. The thing is, this is WRONG. ADCH is defined as volatile, which means that the compiler should NOT be eliminating accesses to it, even if the result is not used...
Simpler version (no arduino code involved):
#include <avr/io.h>
int aRead() {
uint8_t h,l;
l = ADCL;
h = ADCH;
return (h<<8) | l;
}
int main() {
volatile uint8_t x;
x = aRead() ^ 42;
}
Compile with "avr-gcc -mmcu=atmega328p -Os -flto adctest.c" or "avr-gcc -mmcu=atmega328p -Os -flto adctest.c"
It behaves correctly with a simple assignment to x, or with "&255"...
0000008e <main>:
8e: cf 93 push r28
90: df 93 push r29
92: 1f 92 push r1
94: cd b7 in r28, 0x3d ; 61
96: de b7 in r29, 0x3e ; 62
98: 80 91 78 00 lds r24, 0x0078 ; 0x800078 <__TEXT_REGION_LENGTH__+0x7e0078>
9c: 9a e2 ldi r25, 0x2A ; 42
9e: 89 27 eor r24, r25
a0: 89 83 std Y+1, r24 ; 0x01
a2: 80 e0 ldi r24, 0x00 ; 0
a4: 90 e0 ldi r25, 0x00 ; 0
a6: 0f 90 pop r0
a8: df 91 pop r29
aa: cf 91 pop r28
ac: 08 95 ret
westfw:
This looks like a bug. Perhaps in the C compiler itself...
Close. The bug is in the Arduino Core. (And I have complained endlessly about the potential failure.) They gave the compiler / optimizer permission to split / eliminate the reads...
uint8_t low, high;
...
low = ADCL;
high = ADCH;
...
return (high << 8) | low;
With high never used and tied to a volatile byte the optimizer is free to discard everything associated.
The solution is trivial: use ADC. By using the larger volatile datatype the compiler's / optimizer's hands are tied. This becomes the bottom half of analogRead...
...
// ADSC is cleared when the conversion finishes
while (bit_is_set(ADCSRA, ADSC));
return ADC;
#else
return 0;
#endif
}
And I have complained endlessly about the potential failure.
I couldn't find the report in the bug tracker. Any chance you might provide a link to it?
pert:
Any chance you might provide a link to it?
The complaints to the Arduino folks very likely predate Github (e.g. Developers List).
Before GitHub, there was a Google Code bug tracker but all those reports were migrated to the arduino/Arduino GitHub repository's tracker when they did the move. I would hope that a bug ticket would have been opened for any bug reported on the Developers mailing list but maybe that didn't happen. The only thing I found when searching the Developers mailing was this one:
https://groups.google.com/a/arduino.cc/d/msg/developers/Lmj5_knvlPg/V6l3MQXdl3wJ
uint8_t low = ADCL;
return (ADCH << 8) | low;
...is a bad idea. Use the ADC register instead...
return ADC;
- Brian
I don't understand why ADCH being volatile isn't enough. Don't access to volatile memory locations constitute "memory barriers"?
Note that the problem only seems to happen with XOR... If I replace with |, or &, or +, it still optimizes the actual operation to one byte, but it DOES do the read...
pert:
The only thing I found when searching the Developers mailing was this one:
I'm struggling to understand the reason for trying to find such posts or reposting them here.
westfw:
Don't access to volatile memory locations constitute "memory barriers"?
My understanding is that they do not. I recall a grand debate about the topic in regards to AVR-GCC with the conclusion that "memory barrier" is independent of "volatile" with neither implying the other.
westfw:
Note that the problem only seems to happen with XOR...
That is odd.
I'd think it would be obvious. I'd like to understand the problem better and to see why nothing has been done about it. All too often, I see the same discussions of the same topics repeated over and over. It's much more efficient to reference the existing discussions. This also leads to the relevant information being concentrated into one centralized location instead of being fragmented across the years and platforms, never to be reassembled. The best place for that to happen is in the arduino/ArduinoCore-avr repository but the AVR core has only recently been split off from the arduino/Arduino repository
I am one of the administrators of the arduino/Arduino repository. Although I don't have all the authority of the lead developers, I have been able to get quite a few legitimate issues resolved that had been buried in the hundreds of bug reports.
Arduino AVR Boards has recently been moved into its own repository and AVR-specific issues like this need to eventually all be moved from the arduino/Arduino repository to the arduino/ArduinoCore-avr repository. GitHub has recently added a feature that allows issues to be transferred from one repository to another. Now that we have this feature, the biggest work of the transfer is finding all the issues that need to be moved. Toward that end, I've been labeling all those issues with the Architecture: AVR label as I come across them.
pert:
I'd like to understand the problem better and to see why nothing has been done about it.
The simple answer is that nothing has been done because...
I grew tired of tilting at windmills and I seemed to be the only one who was interested in this problem.
Until now, it has been a theoretical problem. At least in the past, the Arduino developers only worked against problems that had reproducible symptoms.
But, an excellent counter argument to "it's only theoretical" is that the current Atmel C / C++ examples all read the conversion value by referencing ADC. The implication is that result = ADC; is the correct choice because that's what the manufacturer does.
I am not convinced that it isn't a compiler bug.
There isn't any way that a code sequence:
l = ADCL;
h = ADCH;
(where both ADCL and ADCH are volatile)
should results in ADCH not being read, regardless of what comes after.
(It seems to be the order of volatile access WRT other access that is subject to re-ordering issues. #5 at this link
We'll see if I get any Compiler Expert comments over on AVRFreaks...
Note that:
return ADCL | (ADCH<<8);
is clearly wrong; there's no way to predict which read should happen first. I don't think the Arduino code has ever had that, though.
Okay, this is starting to blow over my head. However, can I infer that it is an actual Arduino related bug? And therefore can I name it?
I name it XORZ 
Thanks to all you chaps...
It might help looking back to the start of this thread. What brought me here was that a Serial.println(z) causes the behaviour of ^ z to change. It seems that if z is accessed prior to XOR, it behaves as it should. If there is no access, XORZ strikes.
What I now do is:-
unsigned char h = 42;
for (unsigned char j = 0; j < overs; j++) {
unsigned char reading = analogRead(ADC_PORT) % 256;
h = UTable[(reading ^ h)];
}
and it works.
westfw:
I am not convinced that it isn't a compiler bug.
https://www.google.com/search?q=c+volatile+dead+code+optimization
The consensus appears to be "thumbs up for dead code removal regardless of volatile". But, that is hardly a reliable way to answer the question.
westfw:
There isn't any way that a code sequence:l = ADCL;
h = ADCH;
(where both ADCL and ADCH are volatile) should results in ADCH not being read, regardless of what comes after.
Why not? If h is never used the read is dead code.
In any case this definitely falls under the bureaucratic. We need someone well versed in the latest C / C++ standards.
This situation has uncovered an interesting intersection with folks writing security code. They too are faced with "What does volatile do? Will it get that pesky compiler to stop removing this very necessary but appears-to-be-dead code?"
We'll see if I get any Compiler Expert comments over on AVRFreaks...
Excellent!
Note that:
return ADCL | (ADCH<<8);is clearly wrong; there's no way to predict which read should happen first. I don't think the Arduino code has ever had that, though.
That is also true with two separate assignments...
low = ADCL;
high = ADCH;
The optimizer does not have to preserve that order. A memory barrier is needed to force the ordering. That was my original reason for complaining about analogRead.
cossoft:
However, can I infer that it is an actual Arduino related bug?
Yup.
That would be true if ADCL and ADCH were memory addresses, but they're not—they're hardware addresses, and accessing them (even just reading them) performs specific (and required) hardware operations that shouldn't be optimized away or reordered.
Reading from a hardware register should behave just like calling a function - even you don't use the return value, you must still do the operation to ensure any required side-effects occur.
low = ADCL;
high = ADCH;
The optimizer does not have to preserve that order...
All the text I've found about re-ordering accesses to volatile memory have been about reordering the volatile access WRT non-volatile variables. I claim that two volatile accesses can't be re-ordered (because the avr-gcc implementation assumes the reads have side-effects). It does seem to treat it that way almost everywhere else. if I put two consecutive reads of ADCL in there:
int aRead() {
uint8_t h,l;
l = ADCL;
l = ADCL;
h = ADCH;
return (h<<8) | l;
}
It will do two reads. Either the model assumes side effects, or it doesn't. "implementation defined" rather than "undefined." It's a bug that it is behaving inconsistently.
BTW, there's nothing I can think of in the C specifications that guarantees that the bytes of
return ADCW;
would be read in the correct order. 
(they are, and they are required to be read in that order for the 16bit timer(s) as well (which behave differently, but need the same order.))
uint8_t aRead() {
uint8_t h,l;
l = ADCL;
h = ADCH;
return l;
}
does both reads...
westfw:
BTW, there's nothing I can think of in the C specifications that guarantees that the bytes ofreturn ADCW;would be read in the correct order.
There isn't. I recall the AVR-GCC folks make that guarantee. (I also recall they got the ordering wrong in the earlier versions.) I guess that would fall under "implementation defined behaviour".
westfw:
All the text I've found about re-ordering accesses to volatile memory have been about reordering the volatile access WRT non-volatile variables. I claim that two volatile accesses can't be re-ordered (because the avr-gcc implementation assumes the reads have side-effects).
Yup. I agree.
We're left with dead code removal. Any news from AVR Freaks?