[Mesa-dev] [PATCH 1/3] gallivm: Don't use raw_debug_ostream for dissasembling

[Jose Fonseca]

On 29/07/15 13:35, Tom Stellard wrote:
> On Wed, Jul 29, 2015 at 09:54:05AM +0100, Jose Fonseca wrote:
>> On 23/07/15 17:06, Jose Fonseca wrote:
>>> On 20/07/15 21:39, Jose Fonseca wrote:
>>>> On 20/07/15 18:35, Tom Stellard wrote:
>>>>> All LLVM API calls that require an ostream object have been removed from
>>>>> the disassemble() function, so we don't need to use this class to wrap
>>>>> _debug_printf() we can just call this function directly.
>>>>> ---
>>>>>    src/gallium/auxiliary/gallivm/lp_bld_debug.cpp | 27
>>>>> +++++++++++++-------------
>>>>>    1 file changed, 13 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
>>>>> b/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
>>>>> index 405e648..ec88f33 100644
>>>>> --- a/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
>>>>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
>>>>> @@ -123,7 +123,7 @@ lp_debug_dump_value(LLVMValueRef value)
>>>>>     * - http://blog.llvm.org/2010/04/intro-to-llvm-mc-project.html
>>>>>     */
>>>>>    static size_t
>>>>> -disassemble(const void* func, llvm::raw_ostream & Out)
>>>>> +disassemble(const void* func)
>>>>>    {
>>>>>       const uint8_t *bytes = (const uint8_t *)func;
>>>>>
>>>>> @@ -141,7 +141,8 @@ disassemble(const void* func, llvm::raw_ostream &
>>>>> Out)
>>>>>       char outline[1024];
>>>>>
>>>>>       if (!D) {
>>>>> -      Out << "error: couldn't create disassembler for triple " <<
>>>>> Triple << "\n";
>>>>> +      _debug_printf("error: couldn't create disassembler for triple
>>>>> %s\n",
>>>>> +                    Triple.c_str());
>>>>>          return 0;
>>>>>       }
>>>>>
>>>>> @@ -155,13 +156,13 @@ disassemble(const void* func, llvm::raw_ostream
>>>>> & Out)
>>>>>           * so that between runs.
>>>>>           */
>>>>>
>>>>> -      Out << llvm::format("%6lu:\t", (unsigned long)pc);
>>>>> +      _debug_printf("%6lu:\t", (unsigned long)pc);
>>>>>
>>>>>          Size = LLVMDisasmInstruction(D, (uint8_t *)bytes + pc, extent
>>>>> - pc, 0, outline,
>>>>>                                       sizeof outline);
>>>>>
>>>>>          if (!Size) {
>>>>> -         Out << "invalid\n";
>>>>> +         _debug_printf("invalid\n");
>>>>>             pc += 1;
>>>>>             break;
>>>>>          }
>>>>> @@ -173,10 +174,10 @@ disassemble(const void* func, llvm::raw_ostream
>>>>> & Out)
>>>>>          if (0) {
>>>>>             unsigned i;
>>>>>             for (i = 0; i < Size; ++i) {
>>>>> -            Out << llvm::format("%02x ", bytes[pc + i]);
>>>>> +            _debug_printf("%02x ", bytes[pc + i]);
>>>>>             }
>>>>>             for (; i < 16; ++i) {
>>>>> -            Out << "   ";
>>>>> +            _debug_printf("   ");
>>>>>             }
>>>>>          }
>>>>>
>>>>> @@ -184,9 +185,9 @@ disassemble(const void* func, llvm::raw_ostream &
>>>>> Out)
>>>>>           * Print the instruction.
>>>>>           */
>>>>>
>>>>> -      Out << outline;
>>>>> +      _debug_printf("%*s", Size, outline);
>>>>>
>>>>> -      Out << "\n";
>>>>> +      _debug_printf("\n");
>>>>>
>>>>>          /*
>>>>>           * Stop disassembling on return statements, if there is no
>>>>> record of a
>>>>> @@ -206,13 +207,12 @@ disassemble(const void* func, llvm::raw_ostream
>>>>> & Out)
>>>>>          pc += Size;
>>>>>
>>>>>          if (pc >= extent) {
>>>>> -         Out << "disassembly larger than " << extent << "bytes,
>>>>> aborting\n";
>>>>> +         _debug_printf("disassembly larger than %ull bytes,
>>>>> aborting\n", extent);
>>>>>             break;
>>>>>          }
>>>>>       }
>>>>>
>>>>> -   Out << "\n";
>>>>> -   Out.flush();
>>>>> +   _debug_printf("\n");
>>>>>
>>>>>       LLVMDisasmDispose(D);
>>>>>
>>>>> @@ -229,9 +229,8 @@ disassemble(const void* func, llvm::raw_ostream &
>>>>> Out)
>>>>>
>>>>>    extern "C" void
>>>>>    lp_disassemble(LLVMValueRef func, const void *code) {
>>>>> -   raw_debug_ostream Out;
>>>>> -   Out << LLVMGetValueName(func) << ":\n";
>>>>> -   disassemble(code, Out);
>>>>> +   _debug_printf("%s:\n", LLVMGetValueName(func));
>>>>> +   disassemble(code);
>>>>>    }
>>>>>
>>>>>
>>>>>
>>>>
>>>> Series looks good AFAICT.
>>>>
>>>> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
>>>
>>> Tom,
>>>
>>> This broke the profile build.  I fixed with commit
>>> d6b50ba980b733a82fefe2a0f115635a359c445f, but only then I realized that
>>> this change actually causes regression of functionality.
>>>
>>> The debug stream was being used so that we would write the assembly to
>>> /tmp/perf-XXXXX.map.asm files, so that we could later annotate the
>>> assembly intrusctions with the profile hits (via the
>>> bin/perf-annotate-jit).
>>>
>>> But now the assembly is being redirected into the stderr, hence
>>> perf-annotate-jit won't get anything from /tmp/perf-XXXXX.map.asm.
>>>
>>> What was your motivation for this change? General cleanup, or is the
>>> llvm::raw_ostream causing you problems?
>>>
>>> I think that, if I don't reinstate the debug_stream, then I'll need to
>>> replace it with STL streams (std::stringstream and std::ostream).
>>>
>>> Jose
>>
>> Ping?
>>
>> I wanted to make sure I didn't cause you trouble, but if I don't get any
>> answer I'll just assume you don't care either way, and revert the
>> change, as it saves me the effort of trying to replace debug_stream with
>> something else.
>>
>
> Sorry, I wrote a reply but I must not have sent it.  The goal of this patch
> was to eliminate the use of C++ API features in order to reduce the chances of
> the build breaking.  If it is causing problems for you, go ahead and revert it.

I see. I'll see what else can be done then.

Jose