| Message ID | 5217B34F.4090508@markus-regensburg.de |
|---|---|
| State | New |
| Headers | show |
On 08/23/2013 12:09 PM, Tobias Markus wrote: > When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors. > This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS") > > Signed-off-by: Tobias Markus <tobias@markus-regensburg.de> Reviewed-by: Richard Henderson <rth@twiddle.net> > + cpu_fprintf(f, (sc->flags & DESC_B_MASK || > + env->hflags & HF_LMA_MASK) > + ? "DS " : "DS16"); Though we don't have anything in CODING_STYLE that mandates this, IMO expressions shouldn't "unindent" in the middle like this. Better as cpu_fprintf(f, (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK ? "DS " : "DS16")); but that's just me and emacs... r~
On 08/23/2013 10:01 PM, Richard Henderson wrote: > On 08/23/2013 12:09 PM, Tobias Markus wrote: >> When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors. >> This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS") >> >> Signed-off-by: Tobias Markus <tobias@markus-regensburg.de> > > Reviewed-by: Richard Henderson <rth@twiddle.net> > >> + cpu_fprintf(f, (sc->flags & DESC_B_MASK || >> + env->hflags & HF_LMA_MASK) >> + ? "DS " : "DS16"); > > Though we don't have anything in CODING_STYLE that mandates this, > IMO expressions shouldn't "unindent" in the middle like this. Other similar cpu_fprintf()s also "unindent"ed, so I aimed for consistency. I could submit a seperate patch to properly indent the relevant lines, but that seems like a seperate discussion. (Would be better to add it to CODING_STYLE, but that is obviously non-trivial and there are possibly many more files affected.) > > Better as > > cpu_fprintf(f, (sc->flags & DESC_B_MASK > || env->hflags & HF_LMA_MASK) > ? "DS " : "DS16"); > > but that's just me and emacs... > > > r~ > Alternatively, for readability: + cpu_fprintf(f, (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK) + ? "DS " : "DS16"); The upper line would be 82 characters long. I'm not sure how strictly line width is enforced. (Other lines in that file (target-i386/helper.c) also exceed the width limit.)
On 08/23/2013 03:39 PM, Tobias Markus wrote: > Alternatively, for readability: > + cpu_fprintf(f, (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK) > + ? "DS " : "DS16"); > The upper line would be 82 characters long. I'm not sure how strictly line width is enforced. > (Other lines in that file (target-i386/helper.c) also exceed the width limit.) This instance can get below 80 with one more line wrap: + cpu_fprintf(f, + (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK) + ? "DS " : "DS16");
On 23 August 2013 20:09, Tobias Markus <tobias@markus-regensburg.de> wrote: > When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors. > This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS") If you need to do another version of this patch, it would be nice to (manually) wrap the commit message and your below-the-"---" comments at about column 75. Otherwise whoever commits the patch could maybe fix up the message. thanks -- PMM
diff --git a/target-i386/helper.c b/target-i386/helper.c index bf3e2ac..db2f04f 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -147,7 +147,9 @@ cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, fprintf_function cpu_fprintf, cpu_fprintf(f, " [%c%c", (sc->flags & DESC_C_MASK) ? 'C' : '-', (sc->flags & DESC_R_MASK) ? 'R' : '-'); } else { - cpu_fprintf(f, (sc->flags & DESC_B_MASK) ? "DS " : "DS16"); + cpu_fprintf(f, (sc->flags & DESC_B_MASK || + env->hflags & HF_LMA_MASK) + ? "DS " : "DS16"); cpu_fprintf(f, " [%c%c", (sc->flags & DESC_E_MASK) ? 'E' : '-', (sc->flags & DESC_W_MASK) ? 'W' : '-'); }
When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default operation size bit (D/B bit) is not set for Long Mode Data Segments since there are only Data Segments in Long Mode and no explicit 16/32/64-bit Descriptors. This patch fixes this by checking the Long Mode Active bit of the hidden flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data Segments are logged as "DS") Signed-off-by: Tobias Markus <tobias@markus-regensburg.de> --- v2: * Fix line wrapping as suggested in IRC * Break the line Note that alternatively, Data Segments in Long Mode could be logged as "DS64" to avoid confusion about the missing 64 postfix. (But that would be, strictly speaking, wrong because there are only DSs and no DS16/32/64 in Long Mode.) PS: This is my first contribution to an Open Source Project and I would be very happy about constructive feedback, especially about possible wrong line wrapping. target-i386/helper.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)