[PATCH] D14825: [ThinLTO] Add MODULE_CODE_METADATA_VALUES record

[Teresa Johnson via llvm-commits]

On Thu, Nov 19, 2015 at 3:28 PM, Mehdi AMINI <mehdi.amini at apple.com> wrote:
> joker.eph accepted this revision.
> joker.eph added a comment.
> This revision is now accepted and ready to land.
>
> LGTM (with one question)
>
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1924
> @@ -1916,1 +1923,3 @@
>    unsigned NextMDValueNo = MDValueList.size();
> +  if (ModuleLevel && SeenModuleValuesRecord) {
> +    // Now that we are parsing the module level metadata, we want to restart
> ----------------
> I got a typo in my question, I meant: is it possible to have `NextMDValueNo != 0` and *not* `SeenModuleValuesRecord`?
>
> In the end my question is: do we need to check `SeenModuleValuesRecord` here? Is there a difference between this check and the following:
>
> ```
> unsigned NextMDValueNo = ModuleLevel ? 0 : MDValueList.size();

Yes, for very old debug info (the kind we eventually strip via
StripDebugInfo), I found that there are existing MDValueList entries
when we come into this routine to parse module-level metadata. I'm not
sure how the old debug info metadata sections were structured, but I
don't think we should make any assumptions for old bitcode/debug
metadata. So I think I should leave the guard around doing this only
when SeenModuleValuesRecord which guarantees that we are doing this on
new bitcode. Without this record we can't delay the lazy reading of
the module-level bitcode until after function-level bitcode anyway, so
no reset is needed here.


> ```
>
> (Not saying you should write it this way, but what about `if (ModuleLevel) {`)
>
>
>
>
>
> http://reviews.llvm.org/D14825
>
>
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413