Z88dk: Upcoming SDCC 3.8.0 release

Created on 14 Sep 2018  路  37Comments  路  Source: z88dk/z88dk

SDCC developers are working on a SDCC 3.8.0 release. There will probably be a RC1 next week followed by a release the week after. If any serious issues are found in SDCC, the release might be delayed.

I guess now and next week would be a good time for z88dk developers to test current SDCC trunk to see if there are any serious regressions or other bugs that affect z88dk.

Philipp

enhancement help wanted zsdcc

All 37 comments

There is now a release candidate 1; unless issues that important enough to delay the release are found, the final 3.8.0 release will be in 10 days.

Philipp

I still had subtle bugs in the last update I tried but I think they may be in our peephole rules. I think the way to move forward is to get sdcc's regression tests running with our version of zsdcc to help locate where these problems may be.

@aralbrec
Issue #817 is not related to peephole rules: it is fixed by --max-alloc but it is present with no optimization. It may be related to some ZX81 routines, though.
Issue #819 is only occuring if sdcc is used with --opt-code-size. Only some targets seem impacted though (vz200, svi and maybe others).

I found a bug in sdcc z80 peephole rule 10:

replace restart {
    ld  %1, %2
    ld  %3, %4
    ld  %5, %1
} by {
    ld  %5, %2
    ; peephole 10 loaded %5 from %2 directly instead of going through %1.
    ld  %3, %4
} if canAssign(%5 %2), notVolatile(%1), operandsNotRelated(%1 %4), operandsNotRelated(%1 %3), operandsNotRelated(%4 %5), notUsed(%1), notSame(%3 %4 '(hl)' '(de)' '(bc)'), notVolatile(%5)

This is being erroneously applied to this code fragment:

ld a,(hl)
ld hl,(#_foo)
ld h,a

with:

%1 = a
%2 = (hl)
%3 = hl
%4 = (#_foo)
%5 = h

I'm testing a replacement with different condition:

if notUsed(%1), canAssign(%5 %2), notVolatile(%1), notVolatile(%5), operandsNotRelated(%1 %3), operandsNotRelated(%1 %4), operandsNotRelated(%2 %3), operandsNotRelated(%3 %5), operandsNotRelated(%4 %5), notSame(%3 %4 '(hl)' '(de)' '(bc)')

That looks like a serious issue, maybe serious enough to delay the release. operandsNotRelated(%3 %5) should be sufficient to fix it.
Do you have a small, compile able C code sample to reproduce the issue (from which I could make a regression test)?

Philipp

The code fragment is this:

typedef unsigned char type8;

extern void set_info(type8 b) __z88dk_fastcall;
extern void set_arg1(void);
extern void set_arg2_nosize(type8 use_dx, type8 b);
extern void do_bop(type8 b, type8 a);

extern type8 byte1;
extern type8 byte2;
extern type8 *arg2;

void check_btst(void)
{
    set_info((type8) (byte2 & 0x3f));
    set_arg1();
    set_arg2(1, byte1);
    do_bop(byte2, arg2[0]);
}

The peephole rule is eliminating the second byte parameter load arg2[0] in the final do _bop() call.

5603  1A5A              _check_btst:
5604  1A5A  3A 6D 00        ld  a,(_byte2)
5605  1A5D  E6 3F           and a,0x3f
5606  1A5F  6F              ld  l, a
5607  1A60  CD 30 0B        call    _set_info
5608  1A63  CD 75 0B        call    _set_arg1
5609  1A66  3A 6C 00        ld  a,(_byte1)
5610  1A69  57              ld  d,a
5611  1A6A  1E 01           ld  e,0x01
5612  1A6C  D5              push     de
5613  1A6D  CD E0 0F        call    _set_arg2
5614  1A70  F1              pop af
5615  1A71  2A 6D 00        ld  hl, (_byte2)   ;;; arg2[0] is gone
5616  1A74  E5              push     hl
5617  1A75  CD 09 1A        call    _do_bop
5618  1A78  F1              pop af
5619  1A79  C9              ret

This can't be duplicated in sdcc alone as there is interaction with other z88dk rules that changes the code but sdcc peephole 10 does the erroneous transformation.

With peephole 10 eliminated (do_bop call only):

19728 39CD              ;   genAssign
19729 39CD              ;fetchPairLong
19730 39CD              ;fetchLitPair
19731 39CD              ;fetchLitPair
19732 39CD              ;fetchLitPair
19733 39CD              ; peephole z88dk-216c
19734 39CD              ; peephole z88dk-171
19735 39CD              ;   genPointerGet
19736 39CD  2A 00 00        ld  hl,(_arg2)
19737 39D0  7E              ld  a,(hl)
19738 39D1              ; peephole z88dk-96b
19739 39D1              ; peephole z88dk-339a
19740 39D1              ; peephole z88dk-366
19741 39D1              ; peephole z88dk-339a
19742 39D1              ;   genIpush
19743 39D1              ; peephole 1 removed dead load from a into b.
19744 39D1              ; peephole z88dk-391a
19745 39D1              ;   genIpush
19746 39D1              ;fetchLitPair
19747 39D1              ;fetchLitPair
19748 39D1              ; peephole 17 loaded a from (#_byte2) directly instead of using hl.
19749 39D1  2A 00 00        ld  hl,(_byte2)
19750 39D4  67              ld  h,a
19751 39D5              ; peephole 1 removed dead load from l into a.
19752 39D5  E5              push    hl
19753 39D6              ; peephole z88dk-481q1
19754 39D6              ;   genCall
19755 39D6  CD 65 39        call    _do_bop
19756 39D9  F1              pop af
19757 39DA              ;   genLabel
19758 39DA              ; peephole 149 removed unused label l_check_btst_00101$.
19759 39DA              ;   genEndFunction
19760 39DA  C9              ret

Peephole 10 transforms:

19737 39D0  7E              ld  a,(hl)
19749 39D1  2A 00 00        ld  hl,(_byte2)
19750 39D4  67              ld  h,a

to

ld h,(hl)
ld hl,(_byte2)

which is incorrect.

So I think it would be unusual for sdcc-generated code to run into this bug but it is a bug. Looking at the file this comes from as a whole, peephole 10 is probably applied (correctly) ten times in other places.

The fix won't make it into 3.8.0 (but is in the "next" branch, to be merged immediately after the release).

See also https://sourceforge.net/p/sdcc/bugs/2814/

Philipp

As a reference point z88dk-libraries FATFS library, compiled on August 31 had an error which has been apparent since my beginning with this library on Z80 machines. Compiling with the same zsdcc #9958 binary and same command line, the library now does not exhibit the same error. I think this S03 peephole bug fix has addressed a long standing issue. :+1:

Background: reading and listing ls in the root directory of a FATFS drive worked, but listing a sub-directory would result in a random number of files with random file names. That behaviour has now ceased, and correct sub-directory listings are obtained.

Is that the only problem with fatfs you have noticed? I am thinking about splitting it up into functions and incorporating it into the library.

Yes. I've found the ChaN FatFS libraries very stable, and usually without issues on the AVR (gcc) platform. Generally, I've not changed them at all from the original source.

With the SDCC I was having the issue that seemed to be resolved by the above fix. Beyond a recompilation with the new z80asm object version and the patch above, there were no changes that I'm aware of.

My only remaining issue with YAZ180 is that I've not allocated enough heap to permit changing directories, so the cd function returns with "Not enough core". But, copying files into and out of sub-directories now works too. Edit: stupid error on my side.

When the libraries were first established, we made some small MACROs to optimise on little ended Z80, and I've adjusted which functions get built for the RC2014 in read only mode, but that is all.

3.8.0 was released a while ago.
Today, upstream SDCC has enabled regression testing for the z80 backend with --reserve-regs-iy, all tests pass. Essentially, at this point, the code generated with --reserve-regs-iy should be as bug-free as the one wtihout.

Philipp

Assuming that SDCC is now correct, I've prepared a patch file for Version 3.8.2 #10649
I've tested it by compiling ChaN FatFS, and I am using it for RC2014 CP/M-IDE and YAZ180 yabios.

sdcc-z88dk-feilipu.patch

Most of the changes are to support the z88dk specifics, but there are one or two issues to be resolved in sdcc.

Z80 in argument search needs to be offset by 3 bytes (not 4 bytes).

  if (!IS_GB && !IS_RAB && ISINST(pl->line, "in"))
    return(!strstr(strchr(pl->line + 3, ','), "(c)") && (*what == 'a') || strstr(strchr(pl->line + 3, ','), "(c)") && ((*what == 'b') || (*what == 'c')));

Z180 out0 argument search needs to be offset by 5 bytes (not 4 bytes).

  if(IS_Z180)
  {
    // snip
    if (ISINST(pl->line, "out0"))
      return(argContPrec(pl->line + 5, what, 2));
  }

Changing the name of _memcpy to ___memcpy breaks when sdcc libraries are not used.

  /* Now we can be sure to have found a builtin function. */

  if ((!strcmp (bif->name, "__builtin_memcpy") || !strcmp (bif->name, "__builtin_strncpy") || !strcmp (bif->name, "__builtin_memset")) &&
    IS_OP_LITERAL (IC_LEFT (lastparam)) && !operandLitValue (IC_LEFT (lastparam)))
    {
      /* We have a builtin that does nothing. */
      /* TODO: Eliminate it, convert any SEND of volatile into DUMMY_READ_VOLATILE. */
      /* For now just convert back to call to make sure any volatiles are read. */

      strcpy(OP_SYMBOL (IC_LEFT (icc))->rname, !strcmp (bif->name, "__builtin_memcpy") ? "_memcpy" : (!strcmp (bif->name, "__builtin_strncpy") ? "_strncpy" : "_memset"));
      goto convert;
    }

  if ((TARGET_IS_Z80 || TARGET_IS_Z180 || TARGET_IS_RABBIT) && (!strcmp (bif->name, "__builtin_memcpy") || !strcmp (bif->name, "__builtin_strncpy") || !strcmp (bif->name, "__builtin_memset")))
    {
      /* Replace iff return value is used or last parameter is not an integer constant. */
      if (bitVectIsZero (OP_USES (IC_RESULT (icc))) && IS_OP_LITERAL (IC_LEFT (lastparam)))
        return;

      strcpy(OP_SYMBOL (IC_LEFT (icc))->rname, !strcmp (bif->name, "__builtin_memcpy") ? "_memcpy" : (!strcmp (bif->name, "__builtin_strncpy") ? "_strncpy" : "_memset"));
      goto convert;
    }

Since the patch file is pretty short, perhaps it can be further tested and reviewed, please?
I've done what I can.

I have a few questions on the patch:

1) There are a few changes that seem to be only cosmetic. How about removing them to keep the diff small (and thus the patch more maintainable)?

1a) Indentation change in src/SDCCmain.c on the line

~~
fprintf (stream, "published under GNU General Public License (GPL)n");
~
~

1b) Moving of variable declaration for lic, ric, litval in src/SDCCopt.c

1c) Reordering of conditions in src/SDCCpeeph.c:

~~
while ((*bp != 'n' && *bp != '}') && *bp)
~
~

1d) Adding of {, } around a 1 -line else-block in src/SDCCpeeph.c.

1e) TRUE vs. true in z80SurelyReturns() in src/SDCCpeeph.c.

2) SDCC intentionally uses __memcpy() for the implementation of memcpy(): When string.h is not included, the C standard allows the programmer to use a static (but not extern) identifier "memcpy". So when we want to insert a call to memcpy() for struct assignment, we can't just call whatever symbol the name memcpy resolves to: It might be a static variable or function unrelated to memcpy() from the standard library. How about making the change to __memcpy in z88dk libraries, too?

Philipp

Edit: Fixed two typos.

OK. I've done another version that changes just the minimum.

sdcc-z88dk-feilipu-lite.patch

Although since z88dk doesn't use the sdcc peep.c the changes there could also be removed. I'd prefer to leave them in. As noted by @aralbrec in #1014 the order of examination of the opcodes was not completely correct, so I've reordered them from most specific (longest) to shortest.

Bump @spth are the edits problematic for the current sdcc release?
It would be good to know whether they cause any of your tests to fail.

@aralbrec the diff loses nothing from the current patch, but the question is whether the 3.8.2 release has added any regressions? I guess that only shows up (much) later.

3.8.2 is a development version, not a release. The latest release is 3.8.0. If you want a low risk of regressions, the 3.8.0 release would be a better choice.

Philipp

I've just tried the patch on current SDCC 3.8.3 #10679. I have not rebuild the libraries. In that setup, the regression test pass for z80 (both with and without --reserve-regs-iy), except for two that report a linker error. So in general, this looks quite good.

Philipp

@spth if the patch passes your tests (and is otherwise benign), then should I remove the 2 z88dk specifics below, and submit it into the SDCC bug tracker, with correct chunk offsets for #10679?

  • _memcpy to ___memcpy breaks when sdcc libraries are not used
  • z88dk specific version info

Well, those two tests failing at link time should be looked into. And one should test also the effect on the libraries, and the other z80-related backends (z180, gbz9, r2k, r3ka, tlcs90). Testing is easy: Apply the patch to a current source tarball or svn. Build sdcc. Build the regression tests.

SDCC has a patch tracker separate from the bug tracker (https://sourceforge.net/p/sdcc/patches/). For submission there, it would probably be good to split the patch into thematic bits, instead of one big patch.

The two link tests are probably due to the ___memcpy change. They don't appear with the _memcpy patch removed.

I've divided the patch into three parts 1. for z88dk short calls, 2. for z88dk special functions, and 3. for rearranging the peephole optimiser.

sdcc-z88dk-feilipu-peephole-10704.patch
sdcc-z88dk-feilipu-shortcall-10704.patch
sdcc-z88dk-feilipu-specialfunctions-10704.patch

There is a failure of one regression test in bug-3495411.c. I think the failure is because an EXTERN is being handled differently. Not sure what the correct method should be.

This is the offending patch, I believe...

Index: src/SDCCglue.c
===================================================================
--- src/SDCCglue.c  (revision 10704)
+++ src/SDCCglue.c  (working copy)
@@ -365,11 +365,15 @@
               emitDebugSym (&map->oBuf, sym);
               dbuf_printf (&map->oBuf, "==.\n");
             }
-          if (IS_STATIC (sym->etype) || sym->level)
-            dbuf_tprintf (&map->oBuf, "!slabeldef\n", sym->rname);
-          else
-            dbuf_tprintf (&map->oBuf, "!labeldef\n", sym->rname);
-          dbuf_tprintf (&map->oBuf, "\t!ds\n", (unsigned int) size & 0xffff);
+          if (!IS_EXTERN (sym->etype))
+            {
+              if (IS_STATIC (sym->etype) || sym->level)
+                dbuf_tprintf (&map->oBuf, "!slabeldef\n", sym->rname);
+              else
+                dbuf_tprintf (&map->oBuf, "!labeldef\n", sym->rname);
+
+              dbuf_tprintf (&map->oBuf, "\t!ds\n", (unsigned int) size & 0xffff);
+            }
         }

       sym->ival = NULL;

what happens is that this piece of code from the unpatched version

;--------------------------------------------------------
; ram data
;--------------------------------------------------------
    .area _INITIALIZED
_Sip_pCurLcb::
    .ds 2
;--------------------------------------------------------
; absolute external ram data
;--------------------------------------------------------

looks like this after patching.

;--------------------------------------------------------
; ram data
;--------------------------------------------------------
    .area _INITIALIZED
;--------------------------------------------------------
; absolute external ram data
;--------------------------------------------------------

It looks like z88dk handles EXTERN declarations differently, as this further patch creates a list.

Index: src/SDCCglue.c
===================================================================
--- src/SDCCglue.c  (revision 10704)
+++ src/SDCCglue.c  (working copy)
@@ -1827,9 +1831,12 @@
   /* for all variables in this segment do */
   for (sym = setFirstItem (map->syms); sym; sym = setNextItem (map->syms))
     {
-      /* if it is "extern" then do nothing */
+      /* if it is "extern" then add to the extern table */
       if (IS_EXTERN (sym->etype) && !sym->ival)
-        continue;
+        {
+          addSetHead (&externs, sym);
+          continue;
+        }

       /* eliminate redundant __str_%d (generated in stringToSymbol(), SDCCast.c) */
       if (!isinSet (tmpSet, sym))

Thoughts on how to resolve this?

I've applied the first part of sdcc-z88dk-feilipu-peephole-10704.patch (i.e. the one that adds two functions and handling of comments before the condition of a peephole. I'll have to look into the second part more before applying it.

Philipp

I've applied most of sdcc-z88dk-feilipu-shortcall-10704.patch. I excluded the {} in SDCC.y, which I don't understand, and the extern stuff that might cause a regression.

Philipp

I don't understand the z88dk EXTERN handling process, so I can't comment. I guess @aralbrec would be the best to know.

It may be that this piece of the patches remains outside SDCC because z88dk is simply different, and that is not a problem as I see it. The end goal is not to eliminate the differences, but simply to minimise them.

To avoid doubt, I assume you're going to look at the sdcc-z88dk-feilipu-specialfunctions-10704.patch code for z80/peep.c separately too? It wasn't mentioned, and the code is simple.

Thank you for reviewing the z80/peep.c part of the sdcc-z88dk-feilipu-peephole-10704.patch.

It is a fairly substantial reordering of the rules, and hasn't been reviewed. I did the reordering based on the issue that removal of the /t character from pattern matching requires the matches to be strictly in order longest opcode to shortest opcode. I'm not 100% certain I got it 100% correct.

I want to look at the z80-peephole parts of sdcc-z88dk-feilipu-peephole-10704.patch and sdcc-z88dk-feilipu-specialfunctions-10704.patch, but probably won't get around to doing it this week.

However, that list of functions in sdcc-z88dk-feilipu-specialfunctions-10704.patch seems too z88dk-specifc for SDCC at this point (I don't see a good use for them outside of zsdcc; on the other hand the short calls merged yesterday are at least useful for someone who wants to call z88dk functions from sdcc-compiled code).

Philipp

@spth Philipp, I think I've found an issue present in the r10748 release.
Comparison of the r9958 version against r10748 with the optimiser turned off shows a problem.

This is the fragment of C.

bank_get_abs((int8_t)atoi(args[1]))

This is the correct assembly found in r9958

2212  073A              ;main.c:366: bankLockBase[ bank_get_abs((int8_t)atoi(args[1])) ] = 0x00;
2213  073A  C5              push    bc
2214  073B  6B              ld  l, e
2215  073C  62              ld  h, d
2216  073D  CD 00 00        call    _atoi_fastcall
2217  0740  5D              ld  e, l
2218  0741  54              ld  d, h
2219  0742  C1              pop bc
2220  0743  6B              ld  l, e
2221  0744  CD 00 00        call    _bank_get_abs_fastcall

This is the incorrect assembly in r10748

2177  0730              ;main.c:366: bankLockBase[ bank_get_abs((int8_t)atoi(args[1])) ] = 0x00;
2178  0730  C5              push    bc
2179  0731  6B              ld  l, e
2180  0732  62              ld  h, d
2181  0733  CD 00 00        call    _atoi_fastcall
2182  0736  5D              ld  e, l
2183  0737  54              ld  d, h
2184  0738  C1              pop bc
2185  0739  1E 00           ld  e,0x00
2186  073B  16 00           ld  d,0x00
2187  073D  6B              ld  l, e
2188  073E  26 00           ld  h,0x00
2189  0740  CD 00 00        call    _bank_get_abs_fastcall

And another piece of bad assembly from r10748

3030  0C5F              ;main.c:527: fprintf(output,"Saving Bank %01X to \"%s\"", bank_get_abs((int8_t)atoi(args[1])), args[2] );
3031  0C5F  DD 6E EF        ld  l,(ix-17)
3032  0C62  DD 66 F0        ld  h,(ix-16)
3033  0C65  4E              ld  c, (hl)
3034  0C66  23              inc hl
3035  0C67  46              ld  b, (hl)
3036  0C68  DD 6E ED        ld  l,(ix-19)
3037  0C6B  DD 66 EE        ld  h,(ix-18)
3038  0C6E  5E              ld  e, (hl)
3039  0C6F  23              inc hl
3040  0C70  66              ld  h, (hl)
3041  0C71  C5              push    bc
3042  0C72  6B              ld  l, e
3043  0C73  CD 00 00        call    _atoi_fastcall
3044  0C76  5D              ld  e, l
3045  0C77  54              ld  d, h
3046  0C78  C1              pop bc
3047  0C79  1E 00           ld  e,0x00
3048  0C7B  16 00           ld  d,0x00
3049  0C7D  6B              ld  l, e
3050  0C7E  26 00           ld  h,0x00
3051  0C80  CD 00 00        call    _bank_get_abs_fastcall

Note that this issue doesn't happen everywhere. Just where the result of the calls is an index to an array. The issue seems to be the insertion of the zero setting of e and d registers, without due consideration. The attached files show that it works everywhere else.

Attached two listing files showing the issue.
main_9958_SO0.c.lis
main_10748_SO0.c.lis

Just to confirm that this issue is apparent in the unpatched SDCC version, I compiled the same file with the r10748 repo version.

The problem fragment is this

bank_get_abs((int8_t)atoi(args[

It occurs 17 times in the main.c file, and quite a few of them have the atoi() return value stomped on by spurious ld e,0x00 and ld d,0x00 instructions.
main.c.asm

Out of interest, I removed the int8_t casting. This didn't fix the problem.
main_no_int8_t.c.asm

I've made a bug report #2854 in the SDCC repo too, which has been resolved as of r10754.

Ok. Now with r10757, I've made two patch files.

One for z88dk specifics, that should be all that is required to be the new master patch.
sdcc-10757-z88dk-specifics.patch

One for fixes to the sdcc z80 patch.c file. These should be reviewed by @spth, as it is a fairly substantial reordering of the rules, and hasn't been reviewed. I did the reordering based on the issue that removal of the /t character from pattern matching requires the matches to be strictly in order longest opcode to shortest opcode.
sdcc-10757-z88dk-peep.patch

Generally, imho, the r10757 code looks better than r9958. There has been a lot of improvement with register allocations, before peephole optimisation. Most of the improvement is invisible when using the z88dk specific peephole optimisation, but I guess it is better to have better original code and not have to rely on so much on peephole optimisation.

Are there internal z88dk sync reasons for waiting furtherly with https://github.com/z88dk/z88dk/pull/1024 ?

I've applied most of sdcc-z88dk-feilipu-shortcall-10704.patch. I excluded the {} in SDCC.y, which I don't understand, and the extern stuff that might cause a regression.

I've been busy so haven't been able to respond but will look at these things in the next couple of days.

(continuing from premature posting above)

This change in SDCC.y is for compatibility with more versions of bison. The SDCC.y file as-is is rejected by the version of bison (I think gnu) that I have installed on my windows machine.

Ok. I've done another update on the patch file to cover the issue with GNU Bison in SDCC.y
sdcc-10788-z88dk-specifics.patch.txt

Index: src/SDCC.y
===================================================================
--- src/SDCC.y  (revision 10788)
+++ src/SDCC.y  (working copy)
@@ -185,6 +185,10 @@
           cleanUpLevel (SymbolTab, 1);
         }
    | addressmod
+        {
+            /* Reported by @aralbrec that GNU Bison requires this.
+             */
+        }
    ;

The attached patch file includes this fix, so if @spth updates SDCC.y, then we can delete it again.

Given the remaining patch is simply specifics for Z88DK, and all the peephole repair stuff has been submitted separately to SDCC patch tracker, would it make sense to try to get onto SDCC 3.8.x as default?

I'm still trying to find out why the {} is needed, as I never encountered a bison / yacc requiring it. It seems odd to only require it on this rule, but not others. Nevertheless, the workaround seems to be useful to some, so I'd apply it before 3.9.0 even if I don't really understand the issue by then: https://sourceforge.net/p/sdcc/bugs/2858/

Philipp

I'd recommend to base zsdcc by default on a release version of SDCC (i.e. x.y.0) as those tend to contain fewer bugs. SDCC 3.9.0 might be released relatively early in 2019. See https://sourceforge.net/p/sdcc/wiki/SDCC%203.9.0%20Release/ for the current state.

Philipp

Philipp

Probably then, a re-Title of this issue is appropriate.
:stuck_out_tongue_closed_eyes:

Added PR #1071 which will allow some more robust testing of r10892 and (hopefully no) issues to be reported, prior to finalising 3.9.0.

@spth with PR #1071 and #1074 the migration to sdcc 3.8.5 is complete and wrapped.

Let鈥檚 hope that bugs to 3.9.0 are benign and rare.

Noted sdcc bug #2875 that was resolved from r10960.

Subsequent IO port reads are fairly common, this one is a relevant fix.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

aralbrec picture aralbrec  路  5Comments

Fortyseven picture Fortyseven  路  13Comments

suborb picture suborb  路  13Comments

feilipu picture feilipu  路  13Comments

feilipu picture feilipu  路  13Comments