Rockbox

Tasklist

FS#11399 - Data abort in text_viewer

Attached to Project: Rockbox
Opened by Jan Arban (hp_sebastian) - Sunday, 13 June 2010, 13:41 GMT
Last edited by Rafaël Carré (funman) - Wednesday, 23 June 2010, 13:56 GMT
Task Type Bugs
Category Plugins
Status Closed
Assigned To Yoshihisa Uchida (Uchida)
Operating System Another
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

When I'm trying to open a text file with text_viewer, I get the following error:

Data abort
at 308007F4
FSR 0x8
(domain 0, fault 8)
address 0x9689BB14

Player: Sansa Fuze v2
Revision: 26826
This task depends upon

Closed by  Rafaël Carré (funman)
Wednesday, 23 June 2010, 13:56 GMT
Reason for closing:  Accepted
Additional comments about closing:  r27089
Comment by Luca Leonardo Scorcia (LucaLeonardoScorcia) - Monday, 14 June 2010, 20:23 GMT
Me too! I have a reproducible Data Abort in Text Viewer:

Data abort
at 308007F4
FSR 0x8
(domain 0, fault 8 )
address 0x9689B686

The file system is ok, just checked, and I can open the file fine using the Text Editor plugin.
I'm using a FuzeV2 with r26384.
Comment by Michael Gentry (Confuseling) - Tuesday, 15 June 2010, 10:53 GMT
Also seeing this on a Sansa Clip+

I realise they're internally quite similar, but hope this helps anyway;

Data abort
at 30800520
FSR 0x8
(domain 0, fault 8)
address 0x64BA47DE

Rockbox r26849. Reproducible with the first and last numbers changing.

Thanks.
Comment by Rafaël Carré (funman) - Wednesday, 16 June 2010, 06:56 GMT
30800520 is "solidblock" in LCD driver, the other is lcd_bitmap_part

works fine with arm-elf, breaks with arm-elf-eabi
Comment by Rafaël Carré (funman) - Wednesday, 16 June 2010, 09:05 GMT
reproducible on clipv2, not on nano1g/nano2g according to Saint

text_viewer looks like java code: getters, setters, callbacks, file separation is cool but not when each file is calling functions in each other.
In other words the code is difficult to read.

preferences handling looks overly complex and is buggy, but the bugs I've seen so far don't apply to initial loading so the problem is somewhere else.

Note the problem could be in core rockbox if text_viewer uses a feature in some unusual manner, so we would notice it only when using text_viewer.
Comment by Daniel Kalle (DonDan) - Thursday, 17 June 2010, 16:23 GMT
Same here on opening a text file with the text_viewer:

Data abort
at 3080007F4
FSR 0x8
(domain 0, fault 8)
address 0x9089B82C

This happens only with the eabi compiled version!
If I compile it with the --no-eabi option the bug doesn't show up and the text viewer works fine.

Using r26887, Fuze V2

Hope it helps to spot the bug.
Cheers
Comment by Daniel Kalle (DonDan) - Thursday, 17 June 2010, 16:29 GMT

> Comment by Rafaël Carré (funman)
> works fine with arm-elf, breaks with arm-elf-eabi

I should first read...and than reply... =-)

Cheers
Comment by Yoshihisa Uchida (Uchida) - Sunday, 20 June 2010, 11:30 GMT
I found the cause of this problem.
When I correct this problem, I will send the patch. Please wait for a while.
Comment by Yoshihisa Uchida (Uchida) - Sunday, 20 June 2010, 11:48 GMT
this problem's patch send. please confirm it.
it will fix for both eabi and non-eabi.
Comment by Rafaël Carré (funman) - Sunday, 20 June 2010, 14:50 GMT
The patch doesn't fix the crashes, so perhaps there is another problem.

I think instead of changing set_option, the enums should be explicitely ints in tv_preferences.h
It took me 2 reads to understand that the value of optiontype was the size of the type

Different solution attached
Comment by Rafaël Carré (funman) - Sunday, 20 June 2010, 15:17 GMT
Also make functions return unsigned and not enums : still crashes
Comment by Rafaël Carré (funman) - Sunday, 20 June 2010, 17:17 GMT
Some other changes:

Remove get_preferences() and use a global pointer
Explicitely make enums start at 0
Use static const tables for footer/header menus
Remove unneeded memset() in set_default_preferences
Move check_header_and_footer() to set_preferences
Some cosmetics to avoid some long lines

It still crashes on Fuzev2
It works on Clipv2 as long as no settings file is present, and if I comment out header_mode/footer_mode in set_default_preferences() -> I need to investigate

I think support for old versions setting files should be just skipped, use the default settings if the file is too old, it will make code simpler
Comment by Rafaël Carré (funman) - Sunday, 20 June 2010, 23:27 GMT
i committed a modified version of this patch but the bug is still there
Comment by Rafaël Carré (funman) - Sunday, 20 June 2010, 23:59 GMT
works fine when built with -O0 , not with -O
Comment by Rafaël Carré (funman) - Monday, 21 June 2010, 00:13 GMT
previous comment was for whole rockbox built with -O0

Building rockbox with normal (-O) flags and text_viewer with -O0 still crashes so the problem might be in rockbox
Comment by Yoshihisa Uchida (Uchida) - Monday, 21 June 2010, 13:06 GMT
funman, your corrections are very wrong. It contradicts the text_viewer's programming policy.
1) preferenecs can be changed directly for each modules. never do this!
for any modules (excepts tv_preferences.c), do not keep possible this. (preferences is constant).
if we need to change member[s] of the global preferences, we use tv_set_preferences().

2) About tv_check_header_and_footer() is included in tv_preferences.c.
Processing concerning the screen display is a role of tv_window not but other modules.
the role of tv_preferences is only managing of preferenecs and notifies other modules when preferences is changed.
The check on header_mode and footer_mode is work that tv_window not but other modules because modules other than tv_winodw do not understand the meaning of header_mode/footer_mode.

I propose to revert r26998.
And, I think that we should correct this problem carefully.


Comment by Yoshihisa Uchida (Uchida) - Monday, 21 June 2010, 14:07 GMT
I send new patch.
But I don't have Fuze v2, I don't confirm on Fuze v2. maybe this bug might still occur.

operations:
1) get the source r26997.
2) copy apps/plugins/text_viewer/* to other folder.
3) get the latest souces
4) copy 2) to apps/plugins/text_viewer.
5) apply my new patch (rb_text_viewer_abend_2.patch)
6) build


P.S. If this problem occur, report data abort's address please.
Comment by Daniel Kalle (DonDan) - Monday, 21 June 2010, 15:01 GMT
Followed the mentioned operations (1 - 6, build with arm-elf-eabi) but got the same error as before:

Data abort
at 3080007F4
FSR 0x8
(domain 0, fault 8)
address 0x9A89AC44

Fuze v2 r27014 with text_viewer from r26997
Comment by Rafaël Carré (funman) - Monday, 21 June 2010, 15:43 GMT
Uchida,

1) If preferences can not be changed directly we can make the pointer const with this patch
2) For header_mode/footer_mode, this avoids doing the callback 2 times:
tv_set_preferences
-> callbacks -> tv_check_header_and_footer -> tv_set_preferences -> callbacks

I think we should not use too much object oriented programming and hide details
Another solution is to not modify header_mode and footer_mode when setting the preferences, but do it when we are reading
the settings in tv_window.c : I can propose a patch if you want.


If you want to revert these changes anyway I am ok because it is your code so you are the boss :)

Do you have a problem with the other changes in this commit ?

If the other changes are ok for you I can send a single patch which reverts the 2 changes you listed.


About the crash I am still not sure if the problem comes from text_viewer : ranma sent me a crashdump on c200v2 I will look if I can find the cause.
Comment by Rafaël Carré (funman) - Monday, 21 June 2010, 17:07 GMT
The crashdump that ranma sent me is:

-> gui_status_bar_draw()
-> lcd_putsxyofs() in firmware/drivers/lcd-bitmap-common.c:190
-> lcd_mono_bitmap_part(src=?, src_x=?, src_y=?, stride=? x=89, y=0, width=6, height=8) in firmware/drivers/lcd-16bit-vert.c:897 (dst == 0x92821554)

(on c200v2 : 132x80 lcd)
Comment by Yoshihisa Uchida (Uchida) - Tuesday, 22 June 2010, 09:20 GMT
funman,

> 1) If preferences can not be changed directly we can make the pointer const with this patch

The problem is to be able to change members of preferences directly.
In files of the latest version, even if preferences has not been changed directly, someone at one time will change it directly.
Permitting it will destroy the basis of the new text viewer.

> 2) For header_mode/footer_mode, this avoids doing the callback 2 times:
> tv_set_preferences
> -> callbacks -> tv_check_header_and_footer -> tv_set_preferences -> callbacks

It is not a bug. It is in design. (I have already known this.)
tv_preferences doesn't have the authority to change header_mode/footer_mode at all.

Because old tv_check_header_and_footer() (i.e., in old tv_window) had changed header_mode/footer_mode, then callback function was executed. This is not wrong at all. If the callback function was executed though header_mode/footer_mode have not been changed, it is a big problem.

When your method is adopted, processing to which the font is changed will be done by tv_preferences.c.
And whenever such a thing is occured, will tv_preferences be changed in the future?
This is very wrong !!
tv_preferences need not know what any members of preferences means.
If the authority of each modules becomes vague, I would rather revive that dirty viewer.rock.

The third, About the changing of buffer dynamic to static.. This is a big problem.
It is necessary to acquire the buffer dynamically by each module.
The idea that only tv_reader uses a lot of buffers is wrong.
The size of the buffer needed in each module will increase in the future as the function of the text viewer increases.


Finally, you had to have uploaded your modifications to the tracker before committing and you should have heard the opinion.
Or, when I was requesting the opinion from the new text viewer, you should have passed a remark if it was possible...
Comment by Rafaël Carré (funman) - Tuesday, 22 June 2010, 09:38 GMT
If we make the pointer point to a const struct it won't be possible to change preferences directly, and tv_preferences.c can write the static structure without using the pointer to const

I will prepare another patch to remove what you don't like and put it here

By the way I found the bug: it was in gui_statusbar_draw(), not in the text viewer and it will likely be fixed shortly
Comment by Rafaël Carré (funman) - Tuesday, 22 June 2010, 10:05 GMT
- make the preferences pointer const pointer to const data so files other than tv_preferences.c can not change it, only read

- restore init functions but with a different prototype: a pointer to the buffer pointer and a pointer to the buffer size
that way, the functions can adjust the buffer start and the buffer size directly without the need for another function parameter
for now, tv_init_reader() is the only user of dynamic buffer but in the future it can change.

- move tv_check_header_and_footer() back in tv_window.c:
Check if header/footer or font needs to be changed and call tv_set_preferences() only once in this case.

--------------------------------------------------------------

The bug has been fixed in r27045, but I'm leaving it opened so we can discuss my commit, unless if you prefer moving the discussion to mailing list or private mail (like you want)

You're right that I should have commented when you requested opinion on the text_viewer, but I only read the code when chasing this bug.
   tv.diff (11.3 KiB)
Comment by Yoshihisa Uchida (Uchida) - Tuesday, 22 June 2010, 11:41 GMT
Because I am working now, the content cannot be confirmed. (I cann't login in IRC).
And because my work's ending very slows, I think that the answer maybe write tomorrow (and I will login in IRC tomorrow), sorry.
Comment by Rafaël Carré (funman) - Tuesday, 22 June 2010, 12:37 GMT
No problem, don't worry :-)
Comment by Rafaël Carré (funman) - Wednesday, 23 June 2010, 12:54 GMT
fix warnings
   tv.diff (11.4 KiB)

Loading...