78+88 in BCD addition missed carry

[felixcpfung]

With the following program, the carry flag is missed:

mvi a,078h
adi 088h
daa
hlt

please check

regards, Felix Fung

[Aanjhan Ranganathan (aanjhan)]

Thanks for reporting the bug. I will look into it.
Assigning to self and setting the bug as triaged.

Could you please mention the gnusim8085 version you are using?

[felixcpfung (felixcpfung)]

The gnusim8085 I am using is 1.3.6, felix

[Onkar Shinde (onkarshinde)]

Here is the description of DAA instruction I found in - http://www.cs.otago.ac.nz/cosc243/lectures/is_detail.pdf
****************
The contents of the accumulator are changed from a binary value to two 4-bit binary coded decimal (BCD) digits. This is the only instruction that uses the auxiliary flag to perform the binary to BCD conversion, and the conversion procedure is described below. S, Z, AC, P, CY flags are altered to reflect the results of the operation.
If the value of the low-order 4-bits in the accumulator is greater than 9 or if AC flag is set, the instruction adds 6 to the low-order four bits.
If the value of the high-order 4-bits in the accumulator is greater than 9 or if the Carry flag is set, the instruction adds 6 to the high-order four bits.
****************

If you run the program in bug description in debug mode, you will find that instruction 'adi 088h' is setting both C and AC flags. Hence when DAA is executed 6 is added to both high-order and low-order bits and both flags are altered, making then 0.

So as per my understanding the behavior is correct here.

[felixcpfung (felixcpfung)]

Thank you for your explaination on the behavior of DAA. Please note that in
BCD addition,
74 + 88 = 162. That means after addition and DAA, the accumulator should
hold 62 and carry flag should be 1 and that is the correct behavior.
Otherwise, 100 is missed and you can never make a correct multi-byte BCD
Addition.

Besides, as I reported seperately, 99+1 after DAA, the accumulator holds A0
and carry flag
is 0 which is also wrong.

Based on the above observation, I believe that there must be a bug exist
when doing DAA
conversion.

Perhaps you should run the hardware and also theck the CPU manual for the
real behavior
of the CPU
Regards, Felix

2010/5/26 Onkar Shinde <email address hidden>

> Here is the description of DAA instruction I found in -
> http://www.cs.otago.ac.nz/cosc243/lectures/is_detail.pdf
> ****************
> The contents of the accumulator are changed from a binary value to two
> 4-bit binary coded decimal (BCD) digits. This is the only instruction that
> uses the auxiliary flag to perform the binary to BCD conversion, and the
> conversion procedure is described below. S, Z, AC, P, CY flags are altered
> to reflect the results of the operation.
> If the value of the low-order 4-bits in the accumulator is greater than 9
> or if AC flag is set, the instruction adds 6 to the low-order four bits.
> If the value of the high-order 4-bits in the accumulator is greater than 9
> or if the Carry flag is set, the instruction adds 6 to the high-order four
> bits.
> ****************
>
> If you run the program in bug description in debug mode, you will find
> that instruction 'adi 088h' is setting both C and AC flags. Hence when
> DAA is executed 6 is added to both high-order and low-order bits and
> both flags are altered, making then 0.
>
> So as per my understanding the behavior is correct here.
>
> --
> 78+88 in BCD addition missed carry
> https://bugs.launchpad.net/bugs/584093
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in gnusim8085: Triaged
>
> Bug description:
> With the following program, the carry flag is missed:
>
> mvi a,078h
> adi 088h
> daa
> hlt
>
> please check
>
> regards, Felix Fung
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/gnusim8085/+bug/584093/+subscribe
>

[felixcpfung (felixcpfung)]

ps, you may refer to page 3-18 and 3-19 of the Intel 8080/8085 assembly
language
programming manual. If you donot have, I can send you a copy of it in pdf
format.

Regards, Felix

2010/5/28 Fung Felix <email address hidden>

> Thank you for your explaination on the behavior of DAA. Please note that in
> BCD addition,
> 74 + 88 = 162. That means after addition and DAA, the accumulator should
> hold 62 and carry flag should be 1 and that is the correct behavior.
> Otherwise, 100 is missed and you can never make a correct multi-byte BCD
> Addition.
>
> Besides, as I reported seperately, 99+1 after DAA, the accumulator holds A0
> and carry flag
> is 0 which is also wrong.
>
> Based on the above observation, I believe that there must be a bug exist
> when doing DAA
> conversion.
>
> Perhaps you should run the hardware and also theck the CPU manual for the
> real behavior
> of the CPU
> Regards, Felix
>
>
>
> 2010/5/26 Onkar Shinde <email address hidden>
>
> Here is the description of DAA instruction I found in -
>> http://www.cs.otago.ac.nz/cosc243/lectures/is_detail.pdf
>> ****************
>> The contents of the accumulator are changed from a binary value to two
>> 4-bit binary coded decimal (BCD) digits. This is the only instruction that
>> uses the auxiliary flag to perform the binary to BCD conversion, and the
>> conversion procedure is described below. S, Z, AC, P, CY flags are altered
>> to reflect the results of the operation.
>> If the value of the low-order 4-bits in the accumulator is greater than 9
>> or if AC flag is set, the instruction adds 6 to the low-order four bits.
>> If the value of the high-order 4-bits in the accumulator is greater than 9
>> or if the Carry flag is set, the instruction adds 6 to the high-order four
>> bits.
>> ****************
>>
>> If you run the program in bug description in debug mode, you will find
>> that instruction 'adi 088h' is setting both C and AC flags. Hence when
>> DAA is executed 6 is added to both high-order and low-order bits and
>> both flags are altered, making then 0.
>>
>> So as per my understanding the behavior is correct here.
>>
>> --
>> 78+88 in BCD addition missed carry
>> https://bugs.launchpad.net/bugs/584093
>> You received this bug notification because you are a direct subscriber
>> of the bug.
>>
>> Status in gnusim8085: Triaged
>>
>> Bug description:
>> With the following program, the carry flag is missed:
>>
>> mvi a,078h
>> adi 088h
>> daa
>> hlt
>>
>> please check
>>
>> regards, Felix Fung
>>
>> To unsubscribe from this bug, go to:
>> https://bugs.launchpad.net/gnusim8085/+bug/584093/+subscribe
>>
>
>

[Onkar Shinde (onkarshinde)]

Felix,

I think there is some confusion here (at least on my part). The values 74 and 88 you have provided in the program are in hex format (not decimal). Where as in your explanation in comment 4 above you are considering them as decimal.
Anyway, as of now Aanjhan is in better position to analyze the bug as he has more knowledge in this regard and he may actually get access to hardware. So I leave the bug to him. :-)

[felixcpfung (felixcpfung)]

Yes, Onkar, I agree with you that there is some confusion. The program I
reported is for BCD addition. The value in Hex format of course represented
a Binary Coded Decimal value. Or DAA is not required.

I have found this DAA bug existing in many software simulator/Emulator
programms.
Perhaps DAA is not that important to the authors.

Anyway thanks for the attention and I am looking forward to the good news.
Regards, Felix

2010/5/28 Onkar Shinde <email address hidden>

> Felix,
>
> I think there is some confusion here (at least on my part). The values 74
> and 88 you have provided in the program are in hex format (not decimal).
> Where as in your explanation in comment 4 above you are considering them as
> decimal.
> Anyway, as of now Aanjhan is in better position to analyze the bug as he
> has more knowledge in this regard and he may actually get access to
> hardware. So I leave the bug to him. :-)
>
> ** Changed in: gnusim8085
> Importance: Undecided => Medium
>
> --
> 78+88 in BCD addition missed carry
> https://bugs.launchpad.net/bugs/584093
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in gnusim8085: Triaged
>
> Bug description:
> With the following program, the carry flag is missed:
>
> mvi a,078h
> adi 088h
> daa
> hlt
>
> please check
>
> regards, Felix Fung
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/gnusim8085/+bug/584093/+subscribe
>

[Aanjhan Ranganathan (aanjhan)]

OK. I had some time and looked into the DAA handling code. IMO, its all messed up and not behaving as it is supposed to. So there is definitely a *bug*.

[Onkar Shinde (onkarshinde)]

Aanjhan,

I am targeting this for 1.3.7. As of now there is no date for 1.3.7. Since the one set in LP is lapsed we might as well fix a bunch of bugs.

[Debjit Biswas (debjitbis08)]

Hi,

I have created a patch for the DAA instruction. I observed that there were 3 basic errors in the DAA code which are as follows:

1. The most significant 4 bits (which is stored in the "high" variable) was being updated before the operation on the lower bits. It should be updated after the operation.
2. The overflow condition was being checked by comparing whether the operation on the 4 bits is producing a value greater than 9. This should compared with 15 (2^4 = 16).
3. The manual states that the Carry flag will be set if there is a overflow after the operation on the higher 4 bits, but it should not be unchanged if there is no overflow. The older code was resetting the flag if there was no overflow.

I am attaching this patch. Can anyone please point to some sample code to test it with? I have tested a few examples I found in the manual, and the of course the code provided in the description of this bug :)

-- Debjit

[felixcpfung (felixcpfung)]

Hi, Debjit,

I couldnot comment on the patch you made because I donot know how to
recompile the simulater. If you could let me have the re-compiled version, I
will test it.

Regards, Felix Fung

2010/6/4 Debjit Biswas <email address hidden>

> Hi,
>
> I have created a patch for the DAA instruction. I observed that there
> were 3 basic errors in the DAA code which are as follows:
>
> 1. The most significant 4 bits (which is stored in the "high" variable) was
> being updated before the operation on the lower bits. It should be updated
> after the operation.
> 2. The overflow condition was being checked by comparing whether the
> operation on the 4 bits is producing a value greater than 9. This should
> compared with 15 (2^4 = 16).
> 3. The manual states that the Carry flag will be set if there is a overflow
> after the operation on the higher 4 bits, but it should not be unchanged if
> there is no overflow. The older code was resetting the flag if there was no
> overflow.
>
> I am attaching this patch. Can anyone please point to some sample code
> to test it with? I have tested a few examples I found in the manual, and
> the of course the code provided in the description of this bug :)
>
> -- Debjit
>
> ** Patch added: "daa.patch"
> http://launchpadlibrarian.net/49670414/daa.patch
>
> --
> 78+88 in BCD addition missed carry
> https://bugs.launchpad.net/bugs/584093
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in gnusim8085: Confirmed
>
> Bug description:
> With the following program, the carry flag is missed:
>
> mvi a,078h
> adi 088h
> daa
> hlt
>
> please check
>
> regards, Felix Fung
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/gnusim8085/+bug/584093/+subscribe
>

[Onkar Shinde (onkarshinde)]

Aanjhan,

Any update on fixing this bug? Debajit has attached a patch already.

[Onkar Shinde (onkarshinde)]

Aanjhan,

Can we also close this bug soon?

[Aanjhan Ranganathan (aanjhan)]

2010-09-24 Aanjhan Ranganathan <email address hidden>

 * src/8085-instructions.c: Fixed the long standing DAA
 bug. Followed http://www.ray.masmcode.com/BCDdaa.html plus had to
 ensure the addition operation on the accumulator must be done
 using the _eef_inst_func_add_i function so as to ensure setting of
 the carry and other flags. Fixes bug LP: #584093.

Test cases tested: Courtesy: http://www.ray.masmcode.com/BCDdaa.html

Example 1:
   mov al,38h ;packed decimal "38"
   add al,45h ;add packed decimal "45"
   daa ;AL = 7Dh -> 83h (with CF clear = 83 packed decimal)

Example 2:
   mov al,88h ;packed decimal "88"
   add al,74h ;add packed decimal "74"
   daa ;AL = 0FCh -> 62h (with CF set = 162 packed decimal)

Example 3:
   mov al,47h ;packed decimal "47"
   add al,69h ;add packed decimal "69"
   daa ;AL = 0B0h -> 16h (with CF set = 116 packed decimal)

[Debjit Biswas (debjitbis08)]

Hi Anjhan,

Here is the what the 8080 manual says about DAA:

...following a two step process:

1. If the least significant four bits of the accumulator is represents a number greater than 9 or if the Auxiliary Carry bit is equal to one, the accumulator is increased by 6. Otherwise no increment occurs.
/*
What the code should do:

if (low > 9 || sys.flag.ac) { sys.reg.a += 6;....
*/

2. If the most significant four bits of the accumulator now[please NOTE the now, after adding 6 to the lower bits] represents a number greater than 9, or if the normal carry bit is equal to one, the most significant four bits are incremented by six. Otherwise no incrementing occurs.

/*
What the code should do:

get the most significant four bits and check the condition specified:
high = sys.reg.a >> 4;
   if (high > 9 || sys.flag.c)

increment the four bits by six:
sys.reg.a += (6 << 4);
*/

If a carry out the least significant occurs during Step(1), the Auxiliary Carry it is set; otherwise it is reset. Likewise, if a carry out of the most significant four bits occur during Step(2), the normal Carry bit is set; otherwise it is unaffected[NOTE].

/*
What the code should do:

In the first condition when we added 6 to the lower bits,
if((low + 6) > 15) sys.flag.ac = 1; else sys.flag.ac = 0;

6 is added because we are not updating the low variable after adding 6 to register a.

Similarly for normal Carry bit.

In the second step:
if((high + 6) > 15) sys.flag.c = 1;

notice that the flag is not unset if condition fails, as stated in the manual.
*/

Please let me know if I missed some logic, or am I using the wrong manual :)

Thanks,
Debjit

[Aanjhan Ranganathan (aanjhan)]

On Sat, Sep 25, 2010 at 12:37 AM, Debjit Biswas
<email address hidden> wrote:
> Please let me know if I missed some logic, or am I using the wrong
> manual :)

I don't think you are wrong. Thanks for the detailed explanation. Is
there a problem or a bug in the way I have patched? Any test case
failing? I fail to see the difference in what you just wrote and how
the code now behaves.

Its 1 AM here so may be my brain is not functioning any more ;-)

--
A

[Aanjhan Ranganathan (aanjhan)]

On Sat, Sep 25, 2010 at 12:37 AM, Debjit Biswas
<email address hidden> wrote:
> if (low > 9 || sys.flag.ac) { sys.reg.a += 6;....
> */
[snip]
> increment the four bits by six:
> sys.reg.a += (6 << 4);
> */

Very important is the fact that direct modification of accumulator
value is *harmful*. Always use the "add" or appropriate function so
that the flags are also appropriately modified.

This needs to be checked throughout the 8085-instructions
implementation. May be we uncover more bugs.

Regards,
Aanjhan

[Debjit Biswas (debjitbis08)]

Hmm, maybe am not seeing the latest changes through the svn web interface. I am in office so dont have access to personal machine. Will surely test the patch once I return home.

-- Debjit

[Sridhar Ratnakumar (srid)]

I suppose having a unittest infrastructure for the instructions could
be very helpful. Perhaps a command-line interface can be exposed, and
then a Python script be used to test the results for regression.

  $ gnusim8085 run --dump-registers mytestprog.asm
  $ gnusim8085 run --dump-memory mytestprog.asm

Or:

  $ echo 'mvi a, b' | gnusim8085 run --dump-memory -

-srid

On Fri, Sep 24, 2010 at 4:36 PM, Debjit Biswas <email address hidden> wrote:
> Hmm, maybe am not seeing the latest changes through the svn web interface. I
> am in office so dont have access to personal machine. Will surely test the
> patch once I return home.
>
> -- Debjit
>
> _______________________________________________
> Mailing list: https://launchpad.net/~gnusim8085-devel
> Post to     : <email address hidden>
> Unsubscribe : https://launchpad.net/~gnusim8085-devel
> More help   : https://help.launchpad.net/ListHelp
>
>

[Onkar Shinde (onkarshinde)]

On Sat, Sep 25, 2010 at 5:06 AM, Debjit Biswas <email address hidden> wrote:
> Hmm, maybe am not seeing the latest changes through the svn web interface. I
> am in office so dont have access to personal machine. Will surely test the
> patch once I return home.

I hope you meant 'git web interface' and not 'svn web interface'.

Onkar
--
Passion - Some people climb mountains - others write Free software.
Don't ask why - the reason is the same.

[Debjit Biswas (debjitbis08)]

 Yes the git web interface.

-- Debjit

On 09/24/2010 11:30 PM, Onkar Shinde wrote:
> On Sat, Sep 25, 2010 at 5:06 AM, Debjit Biswas <email address hidden> wrote:
>> Hmm, maybe am not seeing the latest changes through the svn web interface. I
>> am in office so dont have access to personal machine. Will surely test the
>> patch once I return home.
> I hope you meant 'git web interface' and not 'svn web interface'.
>
>
> Onkar

[Onkar Shinde (onkarshinde)]

On Sat, Sep 25, 2010 at 12:05 PM, Debjit Biswas <email address hidden> wrote:
> Yes the git web interface.
>
> -- Debjit

I can see the changes right here.
http://gnusim8085.git.sourceforge.net/git/gitweb.cgi?p=gnusim8085/gnusim8085;a=summary

Where are you looking? :-)

Onkar
--
Passion - Some people climb mountains - others write Free software.
Don't ask why - the reason is the same.