[cfe-commits] [PATCH] Macro history (de-)serialization.

[Douglas Gregor]

On Sep 24, 2012, at 6:41 AM, Alexander Kornienko <alexfh at google.com> wrote:

> 
> 
> On Fri, Sep 14, 2012 at 7:46 AM, Douglas Gregor <dgregor at apple.com> wrote:
> 
> On Sep 12, 2012, at 1:58 PM, Alexander Kornienko <reviews at llvm-reviews.chandlerc.com> wrote:
> 
> +    // History of macro definitions for this identifier in chronological order.
> +    SmallVector<MacroInfo*, 8> MacroHistory;
> +    while (MI) {
> +      MacroHistory.push_back(MI);
> +      MI = MI->getPreviousDefinition();
> +    }
> +
> +    while (!MacroHistory.empty()) {
> +      MI = MacroHistory.pop_back_val();
> 
> While this will write out all of the macro definitions and undefs, it has a few issues:
> 
>   - There's no way for the reader to know that more than one macro definition is coming, and in fact the reader looks like it will only end up reading the *first* macro definition, rather than the *last* one. I suspect that #define'ing then #undef'ing a macro and defining it again within a PCH file will lead to the wrong macro definition in a client of that PCH file
> 
> This seems to work correctly, at least the test I've added, should have broken if it didn't.

Ah, now I see why. This

	 MacroOffsets[Name] = Stream.GetCurrentBitNo();

keeps updating the offset for each successive macro definition, and since we're writing out the macro definitions in source order, the most recent bit offset gets placed into that table.

Thanks for adding the test, though!

>   - It forces the reader to load in all of the macro history to get to the most recent one, which will be wasted effort in most cases: the vast majority of clients only care about the most recent macro definition and the rest of the definitions could be loaded lazily (e.g., the way we lazily load the body of a function). 
> 
> You mean adding another level of laziness above  ExternalPreprocessorSource::LoadMacroDefinition & Co? Are there reasons to believe it will help to solve more problems than it creates?

With precompiled headers, more laziness means better performance and a lower memory footprint. It's definitely worth doing well. The solution is most likely to turn MacroInfo's PreviousDefinition into a union of a MacroInfo* (if it's been loaded) and either an ID or an offset. See LazyOffsetPtr, which we use in several other places to provide laziness.

>   - This always writes out all of the macro history, even in chained PCH, where some of the macro history might already be in the original PCH. (This kind of issue matters more for modules).
> 
> Any ideas how to fix this?

Typically we track whether the data was loaded from an AST file (e.g., a LoadedFromAST bit on MacroInfo), and then have a way (typically in the AST writer) to determine the pre-existing IDs for data with LoadedFromAST==true. That lets us tie together to different PCH files.

	- Doug