Re: [avr-libc-dev] Fix for strstr.S (revision 2)

[Theodore A. Roth]

On Tue, 10 Dec 2002, Joerg Wunsch wrote:

:) As Theodore A. Roth wrote:
:)
:) > Attached is what I'll probably commit to cvs.
:)
:) OK, here's my version. ;-)

I like yours better. Go ahead and commit it.

<snip>

:) > It turns out that gcc does that for you automagically when you turn on
:) > optimizations, but it doesn't if you don't.
:)
:) How would optimization optimize an assembler file?

If optimizing a C call to strstr() and gcc sees that the second arg string
is a single char, it calls strchr(), otherwise strstr(). Has nothing to do
with what is inside those functions.

:)
:) Anyway, the one-char string is just a pretty good generic C string as
:) well, so the generic handling that applies to all other strings
:) applies to it very much the same.

Agreed.

:)
:) Then i dropped all the useless push/pop instructions.  r18 through r27
:) are call-used registers that may be clobbered by any function.  See
:) 
http://savannah.nongnu.org/download/avr-libc/doc/avr-libc-user-manual/FAQ.html#faq_reg_usage
:)
:) I finally optimized a few other things.  The various #defines are
:) well-suited to obfuscate that the first incoming paramater and the
:) return value of a function are identical, so the assignment in case of
:) a second null string was just an expensive no-op.  Dropped, some more
:) bytes saved. :)
:)
:) Since the stack pops are gone, there's no point in using XJMP to jump
:) to a single ret instruction.  Just use ret directly.  Some more bytes
:) saved. :)
:)
:) The resulting binary is about 30 bytes smaller than Philip's version
:) (on an avr5 architecture).  For avr5, a further instruction could be
:) saved by using movw to assing X to ret_{lo,hi}, but since this would
:) have caused another preprocessor conditional not already covered by
:) macros.inc, i decided that two saved bytes weren't worth it.
:)
:) Finally, i also re-added the redistribution notice (as Ted did).  A
:) file without this notice is implicitly not freely redistributable,
:) thus we need to have the notice.

Agreed.

:)
:) Since this is effectively a complete rewrite, and there's thus no need
:) to adhere to the (sometimes unusual) style of the old file, i also
:) changed some stylistic things:
:)
:) . use no spaces but a tab to indent the opcode
:) . use a tab to separate the operands from the opcode (both are usual
:)   assembler style)
:) . use assembler comments instead C++ comments (helps syntax highlighting,
:)   and is simply more common for assembler files)
:) . use true local labels that won't appear in the linker symbol table
:)   (starting with .L), this also allows to reduce the label name length
:)   since there's no risk of a global name clash
:)
:) I hope you guys don't mind all this. ;-)

Not at all. I did some format changes myself, just not as thuroughly as
you (as usual ;-).

:)
:) > I did notice that your test case is missing a check to make sure that the
:) > returned value is the expected pointer. I've also attached a program which
:) > also tests this.
:)
:) That's fine.  I also noticed the omission, but just verified in
:) simulavr that the returned pointers are what they ought to be.

If you run my test case in simulavr, you get output like this:

  Connection opened by host 0.0.0.0, port 40413.
  writing 0x00 to 0x0038
  writing 0xff to 0x0037
  writing 0x00 to 0x0032
  writing 0xff to 0x0031
  writing 0x00 to 0x0038
  writing 0x01 to 0x0038
  writing 0x02 to 0x0038
  writing 0x03 to 0x0038
  writing 0x04 to 0x0038
  writing 0x05 to 0x0038
  writing 0x06 to 0x0038
  writing 0x07 to 0x0038
  writing 0x08 to 0x0038
  writing 0x09 to 0x0038
  writing 0x0a to 0x0038
  writing 0x0b to 0x0038

Four lines to setup PORTB, DDRB, PORTD, and DDRD. After that, anything to
port D (0x0032) is a failure. Not the best output, but it keeps the test
code (relatively) simple with out having to use printf.

I've been thinking about changing simulavr to have an "exit on break"
option. Thus, test code running in the simulator could issue a BREAK insn
just before return from main to force the sim to halt once the test is
complete. simulavr already handles the BREAK insn, so adding the flag
should be easy.

:) Of course, verifying this in the test suite is better.  I agree with Eric
:) that it's time to start an official test suite...

I'm all for the test suite, but would rather defer it until after the next
release. I think there's plenty to work on until then. ;-)

Ted Roth