why does clang re-evaluate the target info for each input file?

The subject line says it all, I suppose. It seems to me that the target
would be a per-invocation constant. Is state modified that needs to be
refreshed?

clang.cpp:main():
  for (unsigned i = 0, e = InputFilenames.size(); i != e; ++i) {
    const std::string &InFile = InputFilenames[i];
    ...
    <if not a serialized file> {
      Target = TargetInfo::CreateTargetInfo(&triples[0],
                                            &triples[0]+triples.size(),
                                            &Diags);
    }
    ...
  }

If we want to optimize for the serialized case, perhaps we could allocate
the target information lazily?

Thanks,
Sam

Hi Sam,

Originally the Target was computed only once, prior to adding serialization support. Because the changes I made to the driver when I added serialization support were fairly invasive, small inefficiencies like this were (deliberately) introduced. The desired tradeoff at the time was to have my changes be more readable and understandable at the cost of a few things in the driver being slightly less efficient. The hope was that this would help shake out bugs introduced by my changes (of which there were several that we encountered). Certainly some of these inefficiencies can (and should) be fixed. I am fine with introducing optimizations like the one you suggest as long as we also aim to keep the driver readable and maintainable.

Ted

Hi, Ted.

Ted Kremenek wrote:

Originally the Target was computed only once, prior to adding serialization support. Because the changes I made to the driver when I added serialization support were fairly invasive, small inefficiencies like this were (deliberately) introduced. The desired tradeoff at the time was to have my changes be more readable and understandable at the cost of a few things in the driver being slightly less efficient. The hope was that this would help shake out bugs introduced by my changes (of which there were several that we encountered).

That makes sense. Thanks.

Certainly some of these inefficiencies can (and should) be fixed. I am

> fine with introducing optimizations like the one you suggest as long as
> we also aim to keep the driver readable and maintainable.

What got me looking at this was valgrind complaining about target information being leaked. I've looked at this a little closer, and it appears that the leak is in the library code. I should be able to fix that.

It also looks like there are some other things being recalculated unnecessarily, like header-include information. Would you mind if I try to lift those things out of the loop?

Thanks,
Sam

Hi, Ted.

Ted Kremenek wrote:

Originally the Target was computed only once, prior to adding serialization support. Because the changes I made to the driver when I added serialization support were fairly invasive, small inefficiencies like this were (deliberately) introduced. The desired tradeoff at the time was to have my changes be more readable and understandable at the cost of a few things in the driver being slightly less efficient. The hope was that this would help shake out bugs introduced by my changes (of which there were several that we encountered).

That makes sense. Thanks.

Certainly some of these inefficiencies can (and should) be fixed. I am

> fine with introducing optimizations like the one you suggest as long as
> we also aim to keep the driver readable and maintainable.

What got me looking at this was valgrind complaining about target information being leaked. I've looked at this a little closer, and it appears that the leak is in the library code. I should be able to fix that.

Great!

It also looks like there are some other things being recalculated unnecessarily, like header-include information. Would you mind if I try to lift those things out of the loop?

Feel free to refactor anything out of the loop that makes sense.

Thanks so much!

Hi, Ted.

What got me looking at this was valgrind complaining about target
information being leaked. I've looked at this a little closer, and
it appears that the leak is in the library code. I should be able
to fix that.

Hmm... I can see what the problem is, but I'm not sure how it should be
fixed.

As stated in its header file, the TargetInfo class does not take
ownership of the TargetInfoImpl objects it uses. On the other hand, the
primary TargetInfoImpl object can only be set indirectly, by calling the
CreateTargetInfo() function, but there's no way to get a pointer to the
object the function creates.

The solution I'm leaning toward is making the
TargetInfo::AddSecondaryTarget() method private, since it isn't used
anywhere else, and have the TargetInfo class cleanup any TargetInfoImpl
objects it creates and uses. A patch is attached, if you think that's
the way to go.

Thanks,
Sam

TargetInfo-class-takes-ownership-of-TargetInfoImpl-objects.patch (1.69 KB)

Sorry, new patch. This one zaps the comment I alluded to in my last
email...

Sam

TargetInfo-class-takes-ownership-of-TargetInfoImpl-objects.patch (2.05 KB)

Thanks Sam. Applied!