[PATCH] Emit Clang version information into .comment section (Clang's part of implementation) [PART 1]

Rafael EspĂ­ndola rafael.espindola at gmail.com
Mon Sep 30 08:36:09 PDT 2013


On 30 September 2013 11:28, Katya Romanova
<Katya_Romanova at playstation.sony.com> wrote:
>
>   Hi Rafael,
>
>   Thank you so much for the review. I'm not sure I completely understand your question
>   "Do you expect llvm to reason about this in any way?". If you mean whether llvm will use this information in any way, than the answer is "yes". Please refer to the Phabricator review http://llvm-reviews.chandlerc.com/D1729 "Emit Clang version information into .comment section (LLVM's part of implementation) [PART 2]" for information on how llvm is consuming the produced metadata. Also, in that review I discuss why emitting Clang's version info through inline asm with .ident is not such a good solution as emitting it as metadata. If you don't have time to read that review, let me briefly describe the reasons:
>
>   (1) llvm.ident metadata could be used not only by LLVM, but by other consumers that read LLVM IR
>    and want to generate a version information. It is easier for the LLVM IR consumers to understand the metadata than parse and understand inlined assembly.
>   (2) llvm.ident metadata could be produced not only by Clang, but by other producers that generate LLVM IR. Again, producing the metadata is easier then producing inlined assembly.
>   (3) It makes it easier to handle non-elf targets that don't support .ident.

Yes, sorry I missed that thread before reading this one. I think I
agree with the argument that it makes it easier to check where an IL
file comes from. I was thinking only of the final object file.

>   With respect of the changes that you suggested.
>   - I modified the testcase to handle cases where CLANG_VENDOR is defined.
>   - I'm hesitant to spell EmitVersionIdentMetadata() function name with lowercase. All other functions
>   Emit<Something> in CodeGenModule.h and CodeGenModule.cpp are spelled with the first uppercase letter.
>   That's the reason why I did the same. Otherwise, my function will really stand out among
>   all other functions in these files. I think it better to do things consistently and commit "as is", but at some point of time to refactor and change the spelling of *all* of the function names in these files with lowercase.
>   Let me know if you are OK with keeping the name of EmitVersionIdentMetadata() spelled with uppercase first letter.

Yes, that is fine. My preference is to use the new style guide, but
the line for consistency with old code and the new style is fuzzy.

>   I will send an updated patch when I hear back from you.
>
> http://llvm-reviews.chandlerc.com/D1720


Cheers,
Rafael




More information about the cfe-commits mailing list