> On 8 Aug 2019, at 11:33, Pedro Alves wrote: > > On 8/8/19 9:55 AM, Alan Hayward wrote: > >> diff --git a/gdb/stack.c b/gdb/stack.c >> index 0859815baf..c599caf51c 100644 >> --- a/gdb/stack.c >> +++ b/gdb/stack.c >> @@ -1301,7 +1301,7 @@ print_frame (const frame_print_options &fp_opts, >> { >> uiout->field_core_addr ("addr", gdbarch, pc); >> if (get_frame_pc_masked (frame)) >> - uiout->field_string ("pac", " [PAC]"); >> + uiout->field_string ("addr", " [PAC]"); >> } >> else >> uiout->field_string ("addr", "", >> > > ... because I think that this results in MI printing two different "addr" attributes. > > Instead, you'll need to build a string, with e.g., string_printf, > and use uiout->field_string with ui_out_style_kind::ADDRESS style, > so that MI outputs one single "addr" attribute. > > Please try "gdb -i=mi". You can still type CLI commands, so just "(gdb) start" > and running to main, so that GDB prints the frame in the *stop event should > be sufficient to trigger this. stop doesn’t trigger a PAC because it’s only once we reference a function via the link register that the PAC unmasking happens. However, selecting a previous frame does.... and the issues are obvious now: =thread-selected,id="1",frame={level="1",addr="0x00000000004005b0",pac=" [PAC]",func="main3",args=[],file="cbreak-3.c",fullname="/root/cbreak-3.c",line="9",arch="aarch64"} > > BTW, there are two other places where we output the "addr" field > in the file. Do you want to include "[PAC]" in those? If so, > then factoring out the "addr" printing to a separate function > would be appropriate. > Ok, I can do that. > On 8 Aug 2019, at 17:58, Tom Tromey wrote: > >>>>>> "Pedro" == Pedro Alves writes: > > Pedro> Hmm, I had suggested considering MI in the previous iteration, but > Pedro> I was just thinking of including the "[PAC]" text in the > Pedro> "addr" field. If we're adding a new field, then a few extra > Pedro> things need to be considered: > > Pedro> #1 - documentation, both manual and NEWS should mention this new MI field. > > Oops, I forgot about this. Sorry about that. I’ll add something. > > I don't think putting this information into the "addr" field is a good > idea. It's better, IMO, to let MI field names provide the structure, > rather than requiring clients to also parse the values of fields. > > I realize MI isn't 100% clean on this topic, but we can still not make > it worse. > > Pedro> #2 - calling the attribute "pac" makes it architecture specific. > > I don't think this is such a big deal but at the same time any > reasonable name is fine by me. > > Pedro> #3 - The MI attribute is called "pac", and its content is > Pedro> literally " [PAC]". I'd find that odd if I were a frontend author: > Pedro> the content is right aligned with a space, making doing anything with > Pedro> it other than appending it to the address text probably look odd, > Pedro> unless you bake in awareness of the attribute's text... If I saw > Pedro> an attribute named "pac", I'd expect it to be a boolean? At the > Pedro> least, the left space should not be part of the field, I think? > > I think part of the pain here is an internal constraint, namely that the > CLI ui-out wouldn't know to rewrite the boolean value to something else > here. But perhaps that's something that could just be addressed > directly. It looks like fixing the space just requires an additional call to uiout->text (" “). How about I create a new field addr_flags? It would be a generic field into which targets can add whichever fields they want to. I then could add a call to a new function gdbarch_print_addr_flags() which prints the PAC on AArch64 and nothing on all other targets? Alan.&j!z޶ן4b֫rnr