CodePeer Find the Bug Challenge
Challenge 5 – Dead, unused, predetermined and suspicious code
Abstract:
The package Conductor defines an automated conductor for an orchestra, which is updated both periodically every few micro-seconds (entry Advance of task Concert) and each time a sensor detects that a musician plays a new note (entry Start_Of_Note of task Concert).
Have a look at the following code and see if you can spot the errors. Once you think you’ve got them all, click on the “Go CodePeer” button to see how well you matched up to CodePeer’s comprehensive and rapid analysis of the same code.
Error line 97: “high: conditional check raises exception here: checking that Part >= 1″
Here’s how CodePeer came to this conclusion:
Local variable Part is initialized to zero, whereas array Orchestra starts at
index one, hence the mismatch. In fact, the code here is dead, as the test for
the loop will fail at the very first iteration. CodePeer is reporting that an
error would occur if the execution was able to reach this program point.
Error line 114: “high: conditional check raises exception here: checking that Part >= 1 and Part <= num_parts.all"
Here’s how CodePeer came to this conclusion:
Local variable Part is initialized to zero, whereas array Orchestra starts at
index one, hence the mismatch. Contrary to the previous error, the code is not
dead here, due to the different form of the loop test. CodePeer reports the same
error on the following line 115 when accessing field Dur.
Error line 125: “warning: suspicious constant operation (min(…))/96 always evaluates to 0″
Here’s how CodePeer came to this conclusion:
CodePeer computes a range of Score.Duration’First .. Score.Duration’Last – 1 for
local variable Min from its initialization on line 122. Then, dividing Min by
Score.Duration’Last can only result in value zero, which is clearly suspicious
for an operation on non-constant operands. The problem stems from the inversion
of the conversion to fixed-point type Position and the division on line 125, so
that the division occurs in integers here instead of fixed-points.
Error line 156: “medium: condition predetermined because (Dur <= Score.Duration'last) is always true"
Here’s how CodePeer came to this conclusion:
Local variable Dur is of type Score.Duration, hence it cannot exceed
Score.Duration’Last. The problem here is that the programmer used Dur where
Cumul_Dur was expected.
Error line 167: “warning: unused assignment into Dur”
Here’s how CodePeer came to this conclusion:
Dur is a local variable, hence assigning to it is only useful if its value is
read afterwards, which is not the case here. Instead of defining Dur as a local
variable on line 143, it should be defined as a renaming like Pos on line 140.
--------------------
-- file score.ads --
--------------------
package Score is
type Pitch is new Natural range 0 .. 127;
-- MIDI note in the MIDI Tuning Standard, only representing semitones here
type Long_Duration is new Natural;
subtype Duration is Long_Duration range 0 .. 96;
-- Fraction of a measure, 96 representing the complete measure, which
-- allows representing from thirty-second note to whole note for measures
-- with one to four beats
type Position is delta 10.0**(-2) range 0.0 .. 10_000.0;
-- Distance from the start of the score in number of measures
type Instrument is
(Flute, Recorder, Violin, Trumpet, Clarinet, Oboe, Cornet, Flugelhorn,
Piccolo_Trumpet, Piccolo, Alto_Saxophone, Alto_Flute, Viola, Horn,
Alto_Horn, Trombone, Tenor_Saxophone, Bass_Trumpet, Bassoon,
English_Horn, Baritone_Saxophone, Baritone_Horn, Bass_Clarinet, Cello,
Euphonium, Bass_Trombone, Contrabassoon, Bass_Saxophone, Double_Bass,
Tuba, Contrabass_Saxophone, Contrabass_Bugle);
type Interpreter is (First, Second, Third, Fourth, Fifth);
procedure Get_Note
(Inter : Interpreter;
Instr : Instrument;
Pos : Position;
Fwd : Long_Duration;
Pit : out Pitch;
Dur : out Duration);
-- Return the Pitch and Duration of the note starting forward by Fwd from
-- position Pos for the Interpreter of the Instrument
end Score;
------------------------
-- file conductor.ads --
------------------------
with Score; use Score;
package Conductor is
type Positions is array (Positive range <>) of Position;
type Durations is array (Positive range <>) of Score.Duration;
type Part (Max_Players : Positive) is record
Inter : Interpreter; -- Interpreter for this part
Instr : Instrument; -- Instrument for this part
Num_Players : Positive; -- Number of players in 1 .. Max_Players
Pos : Positions (1 .. Max_Players);
-- Current position of each player in the score
Dur : Durations (1 .. Max_Players);
-- Remaining duration of the current note for each player
end record;
type Parts is array (Positive range <>) of Part (5);
task type Concert (Num_Parts : Positive) is
-- Initialize the parts to be played
entry Initialize (Initial_Parts : Parts);
-- Signal that a player starts a note
entry Start_Of_Note
(Index : Positive; -- Index of the Part being played in the array
Num : Positive; -- Number of the player in the Part
Pit : Pitch); -- Pitch of the note
-- Advance the position of all players by a Step, which is expected to
-- be much smaller than the interval between two consecutive notes
entry Advance (Step : Score.Duration);
-- Start of the concert
entry Play;
end Concert;
end Conductor;
------------------------
-- file conductor.adb --
------------------------
package body Conductor is
task body Concert is
Orchestra : Parts (1 .. Num_Parts);
begin
accept Initialize (Initial_Parts : Parts)
do
declare
Part : Integer := 0;
begin
while Part in Orchestra'Range loop
Orchestra (Part) := Initial_Parts (Part);
Part := Part + 1;
end loop;
end;
end Initialize;
accept Play; -- Start the concert
loop
select
accept Advance (Step : Score.Duration)
do
declare
Part : Integer := 0;
begin
while Part < Num_Parts loop
declare
Pos : Positions renames Orchestra (Part).Pos;
Dur : Durations renames Orchestra (Part).Dur;
begin
for Index in Pos'Range loop
declare
-- Do not allow to advance beyond a new note as
-- it is the role of Start_Of_Note to do so
Min : constant Score.Duration :=
Score.Duration'Min (Step, Dur (Index) - 1);
begin
Pos (Index) := Pos (Index) +
Position (Min / Score.Duration'Last);
end;
end loop;
end;
Part := Part + 1;
end loop;
end;
end;
or
accept Start_Of_Note
(Index : Positive;
Num : Positive;
Pit : Pitch)
do
declare
Pos : Position renames Orchestra (Index).Pos (Num);
Inter : Interpreter := Orchestra (Index).Inter;
Instr : Instrument := Orchestra (Index).Instr;
Dur : Score.Duration := Orchestra (Index).Dur (Num);
Expect_Pit : Pitch;
Expect_Dur : Score.Duration;
Cumul_Dur : Long_Duration := Dur;
begin
-- Look for the next note to be played
Get_Note (Inter, Instr, Pos, Dur, Expect_Pit, Expect_Dur);
-- If this note's pitch does not match, suppose that either
-- the player or the sensor missed a note and continue
while Pit /= Expect_Pit and then
-- Look for a note at the appropriate pitch
Dur <= Score.Duration'Last
-- If it cannot be found one measure in advance, give up
loop
Cumul_Dur := Cumul_Dur + Expect_Dur;
Get_Note
(Inter, Instr, Pos, Cumul_Dur, Expect_Pit, Expect_Dur);
end loop;
if Pit = Expect_Pit then
-- A matching note was found, so update the state
Pos := Pos + Position (Cumul_Dur / Score.Duration'Last);
Dur := Expect_Dur;
end if;
end;
end Start_Of_Note;
end select;
end loop;
end Concert;
end Conductor;
See CodePeer in Action!
In this video, AdaCore engineer Yannick Moy walks you through this month’s challenge using CodePeer and GNAT Programming Studio, GNAT Pro IDE.
Anh Vo said:
Hopefully, I am not the only one participating in this challenge :-)
Lines 64, 69 and 70 need to be redesigned in order to make the codes more robust beside the problems pointed out by CodePeer.
The problem will begin for the following task object:
Summer_Concert : Concert (Num_Parts => 6);
A similar problem occurs when rendezvous with entry Start_Of_Note:
accept Start_Of_Note (Index => 6, Num => 6, Pitch => …)
The changes I would make if I was assigned to improve it. That is I replace Positive at lines 64, 69, and 70 by subtype Part_Range and line 62 as defined below.
Max_Part : constant := 5; — change here if Parts change
subtype Part_Range is Positive range 1 .. Max_Part;
type Parts is array (Positive range ) of Part (Max_Part);
Anh Vo said:
I made a typo at the last line of the codes. Here is the correct one.
Max_Part : constant := 5; — change here if Parts change
subtype Part_Range is Positive range 1 .. Max_Part;
type Parts is array (Positive range ) of Part (Max_Part);
Anh Vo said:
I am sure I did type “Positive Range “. But, it is not shown after I submitted. I guess it must be hypertext file :-)
Yannick Moy said:
Hi Anh,
From the stats provided by YouTube, you’re not the only one following the challenges, but it is true you’re more actively contributing, thanks!
I think you are mixing two concepts in the code I wrote, which are the number of parts (a part is, say, first violin or second oboe) and the number of people playing a part (so that 3 people may be playing first violin part).
In the code, I fixed the number of players for each part to be 5 at a maximum. This would be best defined as a named constant in Ada, but I saved a line not doing it. Since it is just an example, conciseness is of some value here.
PS: for your problem with typing a “box” like $lt;>, this is because the replies are partially interpreted as html, partly as text.
Yannick Moy said:
damn, I mistyped <> …
Anh Vo said:
Syntactically, nothing prevents one from declaring
Summer_Concert : Concert (Num_Parts => 6); — or 7, 8, …
which causes Constraint_Error raised at line 89.
By the way, I would replace
declare
Part : Integer := 0;
begin
while Part in Orchestra’Range loop
Orchestra (Part) := Initial_Parts (Part);
Part := Part + 1;
end loop;
end;
by
for Part_Index in Orchestra’Range loop
Orchestra (Part_Index) := Initial_Parts (Part_Index);
end loop;
Yannick Moy said:
No, the values 6, 7 or 9 for the discriminant of type Concert do not cause a Constraint_Error. This discriminant is only constrained to be of Positive type (between 1 and Positive’Last). You are referring here to the limit 5 of the number of players in a part, like I explained in my previous response.
And yes, your proposed modification of the loop is indeed the proper way to code this kind of loop over an array in Ada, and with this kind of loop you cannot have the same problem as the one I show. I did it with a “while” loop precisely to create a bug. :)
Robin James said:
Why does the code look so unstructured when viewed with Internet Explorer 7? L1-46 are blank. On clicking Go CodePeer there is supposed to be an error on L97 but for me it looks like:
– Do not allow to advance beyond a new note as
Yannick Moy said:
Thanks for your report Robin, this should be corrected now.