On 2018-08-24 02:11, John Darrington wrote:
> On Thu, Aug 23, 2018 at 04:35:25PM -0400, Simon Marchi wrote:
> 
>      It is clear now, but somebody doing a git blame to know why 24-bit
>      integer types were added would only find the patch that adds them 
> by
>      itself and wonder who uses that.  A little message like
> 
>        This patch adds 24-bit integer types, used when debugging on the 
> S12Z
>      architecture (added by a later patch in this series).
> 
>      clears that up.  That might looks a bit silly, but I think it 
> helps in
>      the long run.
> 
> I fully agree with you.   I've worked on other projects however had a
> different opinion - they insisted that the checkin comment NOT contain
> any rationale for the change, instead it should just summarize what
> changed.  I find that rather pointless but anyway ....

Well, if you look at our commit history, you'll see we like to be 
verbose :).

>      > It seems that up till now there has been no 24 bit targets, so 
> the
>      > other
>      > two patches as some necessary things to make that possible.
> 
>      Thanks.  Coming back to the code of the patch, I was wondering if 
> these
>      24-bit types are useful or even relevant for any other 
> architecture.
> 
> There most certainly are plenty of 24 bit architectures  especially in 
> the
> embedded world  - just apparently none that gdb currently supports :(
> 
>      Would it work if you only defined the types for s12z 
> architectures,
>      storing the reference in the gdbarch_tdep object?
> 
> My first reaction is that it probably *could* be made to work, but not
> in an elegant fashion.   Somehow I'd have to avoid that gdb ever calls 
> the
> read_encoded_value function.

I'm not sure I understand.  I was only talking about the definition of 
the int24_t and uint24_t types, not the handling of DW_EH_PE_udata3.  
 From what I read, the C99 standard mandates that the 8, 16, 32 and 64 
variants of the intX_t/uintX_t types exist.  Other types (with other 
values of X) would be extensions.  That's why I thought it would make 
sense to define that in the s12z-specific gdbarches only.  In the end I 
don't really mind, but it just looks like the "clean" way to do it and 
doesn't seem really more difficult.  Can you see if the attached diff 
(applied on top of your series) work for you?

And as far as I understand, this is disconnected from the handling of 
DW_EH_PE_udata3.

> I do concede that  adding DW_EH_PE_udata3 might be problematic since
> it's not part of the dwarf standard.  An alternative might be to rework
> the read_encoded_value function to not rely on the dwarf enums (all it
> really cares about is the size of the target's address space.

I'll take a look at that patch (2/3) separately and reply to it.

Simon