Dropping support for toolchains that don't support location builtins?

[Daniel Cheng]

Right now, we have a bunch of #ifdefs in https://source.chromium.org/chromium/chromium/src/+/main:base/location.h to support toolchains where there is no compiler support for getting debug info (file name, function, line/column number) about the caller.

However, there's an increasing trend towards using base::Location::Current() directly as a default arg instead of having the caller pass FROM_HERE.

Should we just drop support extended debug info when location builtins aren't supported, and just consistently use base::Location::Current()? For now, I think we would keep FROM_HERE still—it would just always evaluate to base::Location::Current().

Daniel

[Peter Kasting]

It looks like these builtins are supported on Clang and gcc, so on the two toolchains we even build on, they should work, right? And if someone did try to compile on another toolchain (e.g. MSVC), would your change just mean they don't get very good debug info? Or would it somehow be a(nother) compile break?

PK

[Daniel Cheng]

It shouldn't be a compile break. In theory, all the debug info available is recoverable from the program counter.

The reason this is coming up is because I wanted to explore using C++20's std::source_location to make base::Location objects smaller. I am not sure if that will be mutually exclusive with considering potential binary size improvements to completely drop the filename and line info from base::Location though. I know the absl team has been discussing this internally.

Daniel

 

PK

[Daniel Cheng]

Upon further experiment, I've discovered that FROM_HERE is being used as a default function arg, but it doesn't really work as a default arg when not using the compiler builtins. So I'm just going to remove CreateFromHere() in a followup to avoid the illusion of things working when they really don't.

Daniel

[Gabriel Charette]

Can we just migrate all callers away from explicit FROM_HERE if clang supports it?

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKo0qUmq%3DBRRLy-N9N6f7FYyMvxptzCkLQFR%3DNuBnft3kw%40mail.gmail.com.

[Daniel Cheng]

We could, but there's still quite a few places that manually embed FROM_HERE (e.g. any caller to PostTask). base::Location::Current() is far more verbose. So we'd want to migrate them all to default args.

However:

- the style guide doesn't allow default args on virtual methods

- there was some feedback that we really want 'implicit current' PostTask() and PostTask() with an explicit location to be unique

- there's a lot more PostTask() variants than I originally imagined. PostTask(), PostDelayedTask(), PostNonNestableTask(), PostNonNestableDelayedTask(), PostDelayedTaskAt(), PostCancelableDelayedTaskAt(), et cetera (and I happen to know some more variants are in discussion :). For consistency, would we allow implicit locations and explicit location variants for all of these APIs?

This would be nice to improve but it's (very) low priority and I ended up moving on to other stuff instead.

Daniel

[Bruce Dawson]

There was a change to FROM_HERE landed recently that seemed to cause a temporary size regression (I think the change was reverted or modified, the regression went away) so be on the watch for that when making any changes to this macro.

[Gabriel Charette]

Re. new variants of PostTask: I was hoping to could use the new style (implicit Location) right away...

But I hadn't realized there was a style guide against default args on virtual methods... Think we could make an exception here?

The style guide's argument is that the same can be done with overloads and in the case of virtual methods, resolution is less confusing. But in this case that's not true, the default arg must be resolved at the callsites, can't just have a Location-less overload resolve to the "default".

In the case of TaskRunners the overloads are all in impl details, so the argument about resolution of the default depending on the type doesn't really apply? And absolute worst case, no logic disaster occurs, just bad location, which should be easy to track down if it ever occurs?

[Kyle Charbonneau]

But I hadn't realized there was a style guide against default args on virtual methods... Think we could make an exception here?
The style guide's argument is that the same can be done with overloads and in the case of virtual methods, resolution is less confusing. But in this case that's not true, the default arg must be resolved at the callsites, can't just have a Location-less overload resolve to the "default".

Is there any reason we can't just make all the post task methods with a default argument for location be non-virtual and have them call into a virtual function? That pattern is already used actually, eg. TaskRunner::PostTask() is not virtual and just calls into the virtual PostDelayedTask(). It would just require extending that pattern slightly to something like the following.

bool PostTask(OnceClosure task,

const Location& location = Location::Current()) {

return PostDelayedTaskImpl(location, std::move(task), base::TimeDelta());

}

bool PostDelayedTask(OnceClosure task,

base::TimeDelta delay,

const Location& location = Location::Current()) {

return PostDelayedTaskImpl(location, std::move(task), delay);

}

virtual bool PostDelayedTaskImpl(const Location& from_here,

OnceClosure task,

base::TimeDelta delay) = 0;

There is a bit more to support SequenceTaskRunner/SingleThreadTaskRunner but it seems like it should work.

Kyle

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LKm_vKEBxgS29hce1mD67ME%2BfqsAaRx3n7%3DSicjtqqBTQ%40mail.gmail.com.

[K. Moon]

On the other hand, I think it's reasonable to ask ourselves if we're just contorting the design to satisfy the style guide, or if a limited exception would make sense.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAERSRDBX5-qn2XHeSB8%3DLU6X4xXXJZsB0Gi2U7d2KPmnGQgu9A%40mail.gmail.com.