Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (454 w, 3 d)

Recent Activity

Today

dblaikie added a comment to D104883: [CodeGen] Add ParmVarDecls to FunctionDecls that are created to generate ObjC property getter/setter functions.

Ah - this is an alternative to D104082? OK. Yeah, looks like it'd address the thing we were discussing in D98799. But do you have some more details on why D104082 wasn't the right direction? I thought we found other places that used a default-constructed/null GlobalDecl() for StartFunction` correctly, so I'm wondering why this would be a problem in this particular case(s)?

Thu, Jun 24, 5:06 PM · Restricted Project
dblaikie added a reviewer for D104827: [DebugInfo] Enforce implicit constraints on `distinct` MDNodes: dexonsmith.
Thu, Jun 24, 11:16 AM · Restricted Project
dblaikie added a comment to D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration.

Yeah, all that sounds reasonable to me - @brunodefraine could you look into supporting nodebug in a similar way as @aaron.ballman has described here?

Since the debuginfo for use() is slightly affected by the nodebug version of t1() that follows it, I can see how this back propagation is perhaps dangerous. Checking that nodebug is the same on all declarations of a function is a way to prevent this.

But when discussing the PR, @probinson wrote "I'm inclined to think we want this to work" and I can see what he means from the use case where I observed the bug. If you don't want debuginfo for the implementation of t1(), it should be fine to annotate just the function definition in an implementation file, not the declaration in a header, since the debuginfo of the implementation is not of the caller's concern. But nodebug as it exists does affect the debuginfo of callers as well, so I cannot really express that I don't want debuginfo for the implementation of a function and leave its callers unaffected?

I can see the convenience there, to be sure, being able to put the attribute directly on the function you want to debug - but consistency in how attributes are handled (admitedly this isn't a strong consistency - some are handled this way, some aren't) & consistently seeing the same state for an attribute for a given function seems useful.

@probinson - thoughts?

To me, this is the key bit:

But nodebug as it exists does affect the debuginfo of callers as well, so I cannot really express that I don't want debuginfo for the implementation of a function and leave its callers unaffected?

because it does affect the callers, the programmer introducing the API should be aware of that. Making this case an error helps them to understand that this attribute is actually a part of their API and not an implementation detail, and silently applying the attribute may cause hard-to-debug problems for them after deployment of their API.

Thu, Jun 24, 11:15 AM · Restricted Project

Yesterday

dblaikie added a comment to D99981: [demangler] Support the new Rust mangling scheme (v0).

For the record, support for Rust mangling scheme landed in:

With the only remaining part being a support for non-ASCII identifiers in D104366.

Thanks for review David!

Wed, Jun 23, 8:48 PM · Restricted Project
dblaikie added a comment to D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration.

Yeah, all that sounds reasonable to me - @brunodefraine could you look into supporting nodebug in a similar way as @aaron.ballman has described here?

Since the debuginfo for use() is slightly affected by the nodebug version of t1() that follows it, I can see how this back propagation is perhaps dangerous. Checking that nodebug is the same on all declarations of a function is a way to prevent this.

But when discussing the PR, @probinson wrote "I'm inclined to think we want this to work" and I can see what he means from the use case where I observed the bug. If you don't want debuginfo for the implementation of t1(), it should be fine to annotate just the function definition in an implementation file, not the declaration in a header, since the debuginfo of the implementation is not of the caller's concern. But nodebug as it exists does affect the debuginfo of callers as well, so I cannot really express that I don't want debuginfo for the implementation of a function and leave its callers unaffected?

Wed, Jun 23, 8:47 PM · Restricted Project
dblaikie added a comment to D103131: support debug info for alias variable.

Do you have any interest in seeing if gdb could/would be fixed to handle the imported declaration style? Might be worth knowing if that's feasible, if they have some ideas about if/why that would be a bad idea, etc.

Wed, Jun 23, 12:20 PM · debug-info, Restricted Project, Restricted Project
dblaikie accepted D104736: [LangRef] add note to warn-frame-size about ODR.

Sounds alright

Wed, Jun 23, 12:19 PM · Restricted Project
dblaikie added a comment to D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration.

@aaron.ballman Do we have attributes/infrastructure for attributes that have to be the same from their first declaration or at least from their first call? I'm wondering if it might be simpler/better to require nodebug to be provided earlier, rather than fixing it up after the fact like this.

(for instance, requiring it to be attributed on the declaration would ensure we don't create call_site descriptions for calls to nodebug functions - which would save some debug info size)

We don't have any attributes that have to be the same "from their first call" that I'm aware of. But we do have attributes that need to be the same on all declarations. IIRC, the section and code_seg attributes both have to be specified on the initial declaration in some circumstances, and [[noreturn]] needs to be on the first declaration we see. We don't have any tablegen machinery to enforce this, the logic needs to be manually written. I think it'd live somewhere around Sema::mergeDeclAttributes() or Sema::MergeFunctionDecl() most likely.

Wed, Jun 23, 11:22 AM · Restricted Project
dblaikie added a comment to D104342: [IR] convert warn-stack-size from module flag to fn attr.

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Yeah, I don't think we have a way to add more verbose/custom documentation for diagnostics. (@aaron.ballman might have some ideas)

I'm not aware of any way to do that today -- it would require more machinery for generating the diagnostic documentation, which is made harder by this particular diagnostic being a backend one that's not written out from tablegen.

Wed, Jun 23, 9:03 AM · Restricted Project, Restricted Project
dblaikie added a comment to D103131: support debug info for alias variable.

Here is what we get when we replace int with float.

$lldb-11 ./a.out 
(lldb) target create "./a.out"
Current executable set to '/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 4 at test.c:3:12, address = 0x0000000000400484
(lldb) p oldname
(float) $0 = 1
(lldb) p newname
(void *) $1 = 0x000000003f800000

Yep, looks like it's ignoring the imported declaration in favor of the raw symbol table entry.

How's lldb handle GCC-style debug info (the variable declaration, without any imported declaration) for the alias?

lldb's behavior is same even with GCC -style debug info.

Wed, Jun 23, 8:22 AM · debug-info, Restricted Project, Restricted Project
dblaikie accepted D104784: [LAA] Make getPointersDiff() API compatible with opaque pointers (NFCI).

Sounds good, thanks!

Wed, Jun 23, 7:45 AM · Restricted Project
dblaikie updated subscribers of D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration.

@aaron.ballman Do we have attributes/infrastructure for attributes that have to be the same from their first declaration or at least from their first call? I'm wondering if it might be simpler/better to require nodebug to be provided earlier, rather than fixing it up after the fact like this.

Wed, Jun 23, 7:34 AM · Restricted Project
dblaikie added a comment to D104784: [LAA] Make getPointersDiff() API compatible with opaque pointers (NFCI).

Probably worth some test coverage, if possible - does this allow these transformations to be tested with opaque pointers now, or would we have to resort to unit tests to verify this new functionality?

Wed, Jun 23, 7:29 AM · Restricted Project
dblaikie added a comment to D103131: support debug info for alias variable.

Here is what we get when we replace int with float.

$lldb-11 ./a.out 
(lldb) target create "./a.out"
Current executable set to '/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 4 at test.c:3:12, address = 0x0000000000400484
(lldb) p oldname
(float) $0 = 1
(lldb) p newname
(void *) $1 = 0x000000003f800000
Wed, Jun 23, 7:03 AM · debug-info, Restricted Project, Restricted Project

Tue, Jun 22

dblaikie added a comment to D103131: support debug info for alias variable.

Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable i, another file with debug info with a using ns::i in the global scope - printing i when stepping into the second file correctly prints the using based alias value, but stepping into the file without debug info and printing i complains about not knowing the type of that i)

How's lldb go?

lldb seems to be printing value , but I worry about type.

* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x0000000000400484 a.out`main at test.c:3:12
   1    int oldname = 1;
   2    extern int newname __attribute__((alias("oldname")));
-> 3    int main(){}
(lldb) p oldname
(int) $0 = 1
(lldb) p newname
(void *) $1 = 0x0000000000000001
Tue, Jun 22, 10:57 PM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D103131: support debug info for alias variable.

Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable i, another file with debug info with a using ns::i in the global scope - printing i when stepping into the second file correctly prints the using based alias value, but stepping into the file without debug info and printing i complains about not knowing the type of that i)

Tue, Jun 22, 3:47 PM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D95339: [RFC][test] Adapt debug-info lit framework for more general purposes - part 1.

Seems like there are only a few line changed other than the renaming? What kind of review is needed here? Just an agreement on the principle of moving these? I think it should be someone having a stake in the debug-info tests to give some sort of approval here.

Yeah, exactly. I've not had any positive confirmation that people are happy with the overall idea.

@aprantl @dblaikie maybe?

Tue, Jun 22, 1:02 PM · Restricted Project
dblaikie updated subscribers of D104342: [IR] convert warn-stack-size from module flag to fn attr.

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Tue, Jun 22, 12:55 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D104736: [LangRef] add note to warn-frame-size about ODR.
Tue, Jun 22, 12:52 PM · Restricted Project
dblaikie added a comment to D104551: Delay initialization of OptBisect.

I'm open to that, but it would likely take a while to come up with, and implement an alternative.

Would you be opposed to merging this as a workaround for the time being?

Tue, Jun 22, 9:46 AM · Restricted Project
dblaikie added a comment to D104551: Delay initialization of OptBisect.

Sorry, I was incorrect in my original statement. I have fixed it.

Tue, Jun 22, 8:35 AM · Restricted Project

Mon, Jun 21

dblaikie added a comment to D104619: [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id.

Thanks for having a look!

This'll need a test case

Definitely. Do you have a suggestion for what test suite that should go into? I had a quick look but couldn't find anything that obviously exercised TypePrinter.

Mon, Jun 21, 6:02 PM · Restricted Project
dblaikie added a comment to D104619: [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id.

This'll need a test case & does the change pass all existing tests?
Also, the patch description could use more detail - it can refer to the bug for more context, but there should be enough detail in the patch title/description to understand the general purpose, etc. (and you can shorten the bug reference to "PR50774" rather than the whole bugs.llvm.org URL)

Mon, Jun 21, 5:49 PM · Restricted Project
dblaikie added a comment to D104601: [Preprocessor] Implement -fnormalize-whitespace..

One of the concerns I'd have, for instance (have you done some broad testing of these patches on sizable code bases?) is that it wouldn't surprise me if clang had some scalability bugs/issues with very long source lines - so it might be necessary to introduce some (arbitrary?) newlines to break up the code. Though I'm not sure - no need to do that pre-emptively, but might be good to have some data that indicates whether this might be a problem or not.

Mon, Jun 21, 5:47 PM · Restricted Project
dblaikie added a comment to D104663: [OpaquePtr] Remove checking pointee type for byval/preallocated type.

"arguably incorrect IR" isn't a phrase that is especially comforting. It is or isn't, and should have well defined behavior (including being explicitly undefined, if that's the case). Any chance we can get a clearer statement one way or the other?

Is this an alternative to that patch that kept getting committed/reverted that I think was related to trying to unify call site and function declaration parameter attributes? Might be worth mentioning that one in this patch description for history/context?

There are other attributes that don't seem to do this kind of "try the call instruction or the function declaration", do they? so I'd be a bit hesitant about this direction. Would like to get @rnk's thoughts on this too, perhaps.

As someone who has all the context, the commit message makes sense to me. In the change you allude to, we tried to stand on the principle that a call instruction should carry all of the ABI attributes it needs, but in practice, we found that IR producers, in particular instrumentation passes, usually don't set ABI attributes on call sites. It should be a semantics-preserving transform to rinse the target of a call instruction through an identity function, but in practice, that would break all these IR producers. It would make most direct calls indirect, and the ABI attributes would no longer be correct.

Mon, Jun 21, 5:46 PM · Restricted Project
dblaikie added a comment to D104466: NOT FOR REVIEW: proof-of-concept for building lib/linux/libclang_rt.profile-x86_64.a on a mac in the gn build.

If you create a review using arc's --draft (I think that's the name/spelling) it'll avoid sending email to the *-dev list, so you can create it and share it around without adding more email to the busy *-dev lists until you're ready to send it for review, btw.

Mon, Jun 21, 5:43 PM · Restricted Project
dblaikie added a comment to D104551: Delay initialization of OptBisect.

What situation are you dealing with where OptBisect is created during global constructors? OptBisect uses ManagedStatic which only creates the object on first access, right? So maybe it'd be suitable to change the code so it doesn't try to access OptBisect during global construction?

Mon, Jun 21, 5:42 PM · Restricted Project
dblaikie added a comment to D104668: [OpaquePtr] Handle addrspacecasts in InstCombine.

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

I think PointerType::hasSameElementTypeAs(PointerType *) is good, we already have weird PointerType methods that are only for the opaque pointer transition, like PointerType::getWithSamePointeeType(), that aren't used in a lot of places and are to be deleted after.

Mon, Jun 21, 3:27 PM · Restricted Project
dblaikie added a comment to D104601: [Preprocessor] Implement -fnormalize-whitespace..

This is probably more @aaron.ballman 's wheelhouse, but for my money this seems pretty problematic - will make quoted text in compiler diagnostics weird/difficult to read, etc.

Mon, Jun 21, 3:17 PM · Restricted Project
dblaikie added a comment to D104342: [IR] convert warn-stack-size from module flag to fn attr.

I don't know that there's a good answer (in more extreme cases - like different optimization levels or CPU features, at least at Google our answer has ended up being "compile the whole program with the right CPU features, because there's no great way to support good optimizations while respecting CPU features on a per-function basis"), basically - so this was more a "heads up, this is going to be possibly unavoidably messy/unreliable on the edges".

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

Sure. Let me land this, since we (Google & ClangBuiltLinux) have some tests and builds failing due to https://reviews.llvm.org/D103928. I will then send a follow up for us to iterate on in regards to documenting this more.

(why I think there's no solution to this: any rule (highest wins, lowest wins, mismatch fails to compile) will create surprising/problematic effects, eg: you have one TU with the function in it and some value for the attribute - a new TU could introduce a copy of the function with a higher or lower value - and whichever choice of policy would then cause problems for one case or the other case. (either enforcing a stronger warning level on code that didn't ask for it, or slackening the warning level for code that thought it was protected by the warning))

I agree. I don't think even having a function attribute in C for -Wframe-larger-than would resolve such policy issues either.

Mon, Jun 21, 3:09 PM · Restricted Project, Restricted Project
dblaikie accepted D104342: [IR] convert warn-stack-size from module flag to fn attr.

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

I don't think it's as easy as "don't do that". Unless someone passes exactly the same flags to every compilation (which they won't, that's why this is being implemented as a function attribute) then it'll be really easy for an inline function in a header (say, std::vector<int>::size - something easy for two unrelated translation units to use) in two different translation units each with a different warn-stack-size flag and so to get somewhat arbitrary behavior about how that function is warned on.

Ah, ok that's which language construct can produce linkonce_odr. Fair point.

For instance: maybe the function doesn't get a warning because a copy with a higher warn-stack-size value is picked, until the translation unit using that higher value is refactored and starts using std::list instead of std::vector... and now some other TU's std::vector is picked, with a lower warn-stack-size value and breaks the build (assuming -Werror)...

This seems like a general problem perhaps with linking IR? Perhaps IRLinker::copyFunctionProto or IRLinker::mapAttributeTypes should try to do something here, though I'm not sure yet which policy would be preferred?

Mon, Jun 21, 2:55 PM · Restricted Project, Restricted Project
dblaikie updated subscribers of D104663: [OpaquePtr] Remove checking pointee type for byval/preallocated type.

"arguably incorrect IR" isn't a phrase that is especially comforting. It is or isn't, and should have well defined behavior (including being explicitly undefined, if that's the case). Any chance we can get a clearer statement one way or the other?

Mon, Jun 21, 2:29 PM · Restricted Project
dblaikie added a project to D104628: Fix a not debug invariant issue in GlobalOpt/GlobalStatus: debug-info.
Mon, Jun 21, 2:15 PM · debug-info, Restricted Project

Fri, Jun 18

dblaikie added a comment to D104342: [IR] convert warn-stack-size from module flag to fn attr.

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

Fri, Jun 18, 4:52 PM · Restricted Project, Restricted Project
dblaikie updated subscribers of D98477: [ADT] Add IntrusiveVariant, VariantTraits, and new STLForwardCompat.

@rsmith @dexonsmith @aaron.ballman - folks who might have some opinions on C++ API design. Any thoughts on this patch in general, but also in particular how the discriminator is represented in supported classes? (I'd like to avoid macros, and there are a few tradeoffs around that)

Fri, Jun 18, 12:07 PM · Restricted Project
dblaikie accepted D104362: [Demangle][Rust] Hide implementation details NFC.

Sounds alright

Fri, Jun 18, 12:05 PM · Restricted Project
dblaikie accepted D104342: [IR] convert warn-stack-size from module flag to fn attr.

Sounds OK to me.

Fri, Jun 18, 12:02 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D104271: llvm-dwarfdump: Print warnings on invalid DWARF.
Fri, Jun 18, 11:49 AM · Restricted Project
dblaikie added a comment to D104380: [lldb] Set return object failed status even if error string is empty.

This change would have been part of https://reviews.llvm.org/D103701 if I had realised that this was in fact how it worked. I separated it from the follow ons because of that and to not bury 3 lines of change that apply to all callers of these functions, in 300 lines of change to callers of SetStatus(eReturnStatusFailed). If I were bisecting a failure caused by these changes I'd appreciate the separation but there's probably not much difference.

Fri, Jun 18, 11:41 AM · Restricted Project
dblaikie accepted D104525: [lldb] Assert that CommandResultObject error messages are not empty.

Sounds good, if this already passes all tests/etc (ie: there actually aren't any callers passing empty strings/non-failed errors/etc)

Fri, Jun 18, 11:38 AM · Restricted Project

Thu, Jun 17

dblaikie accepted D104358: [Demangle][Rust] Parse dot suffix.

Looks good, thanks!

Thu, Jun 17, 2:01 PM · Restricted Project
dblaikie accepted D90352: Introduce a Bazel build configuration.

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

Or if there's some part of this review that could be extracted (is there sort of a "null" implementation - maybe literally just the top level utils/bazel directory, empty, or with a README, etc) and committed with that approval, and then invested bazel-using folks could use this review thread for further discussion on the design of the bazel build files?

I'm happy to restructure this patch in that way if others would prefer (check in the README first). I did think about something like that, but I thought that non-Bazel folks might want to review some of the actual substance.

Thu, Jun 17, 2:00 PM · Restricted Project
dblaikie added a comment to D104342: [IR] convert warn-stack-size from module flag to fn attr.

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

I think you meant s/module level/function level/? That's a good question, one I had to think about a little bit. Here's my thoughts on the behavior this should exhibit, please let me know if you agree.

When using -Wframe-larger-than=<threshold> per TU, the developer wants to be alerted if any stack frame exceeds a threshold. The Linux kernel's use case is that the kernel's stack is limited to (usually) two pages (ulimit -s; typically 8KiB, but different architectures do support non-4KiB page sizes), so functions using more than 1KiB of stack are usually indicative of large objects being stack allocated that should have been heap allocated.

Currently, in C (and C with GNU extensions), there is no way to describe to the compiler function-grain specific values for -Wframe-larger-than=; rather than fine grain per function control, we only have coarse grain TU control.

So in the general case (non-LTO), we can only perform inline substitution at call sites visible from callers' definitions. Because there's no GNU C function attribute to change the current value of -Wframe-larger-than=, it's not possible for that value to differ between caller and callee. But with LTO; shit gets weird.

Suddenly now with LTO, we have cross TU (cross Module) visibility into call sites, so we can inline across TU/Module boundaries. Thus we can have an IR intermediary object with a call site where the caller's value of -Wframe-larger-than= differs from the callees! So the question is what should happen in such a case?

The extremely conservative approach which we have done in the past for certain mismatched function attributes is to simply not perform inline substitution, if we have no other options. This adds overhead to the inliner to check the XOR of attribute lists of the caller and callee for each call site.

But I *think* (and am open to sugguestions) that we should:

  1. permit inline substitution
  2. the caller's value of "warn-stack-size"= IR Fn Attr wins

I think this is ok because: if caller is defined in TU1 with -Wframe-larger-than= distinct from callee defined in TU2 with a different value of -Wframe-larger-than=, then we don't care what callee's value was. callee may even be DCE'd if it's inlined into a lone call site. I'd expect in such cases that callee's value was larger than caller's, in which case callee should be attributed no_inline for LTO if the tighter threshold for caller now warns. If callee's value was smaller than callers and we performed inline substitution, I think that's also perfectly fine, caller should not become "more strict."

Generally in the Linux kernel, we see a common value of -Wframe-larger-than= throughout most of the TUs, with only a few generally having a larger value to relax constraints a little. (Also, those relaxations are questionable, given the intent of -Wframe-larger-than= use in the kernel in the first place).

Let me add such a test to encode that intention; though I don't know yet what's involved/possible to implement. Let's see.

Thu, Jun 17, 1:56 PM · Restricted Project, Restricted Project
dblaikie added a comment to D90352: Introduce a Bazel build configuration.

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

Thu, Jun 17, 10:03 AM · Restricted Project
dblaikie added inline comments to D104358: [Demangle][Rust] Parse dot suffix.
Thu, Jun 17, 9:45 AM · Restricted Project
dblaikie added inline comments to rG02c718301b30: llvm-objcopy: fix section size truncation/extension when dumping sections.
Thu, Jun 17, 9:40 AM
dblaikie added a comment to D104380: [lldb] Set return object failed status even if error string is empty.

This might be a case of an overly reduced patch - this patch alone I'd expect to be observable in some way, and tested. If the "instring.empty()" condition never fires, then I'd expect to change that to an assert, rather than changing the semantics of it in an unobservable way.

Thu, Jun 17, 9:31 AM · Restricted Project

Wed, Jun 16

dblaikie added a comment to D104291: [Debug-Info] strict dwarf for DW_LANG_C_plus_plus_14.

There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

Oh - I see, c++03 is defined in LangStandards.def an alias for c++98, as the former essentially consists of bugfixes for the latter. This loosely suggests to me that C++03 implementations are (likely to be / mostly?) conformant to C++98, but that C++98 implementations may not be fully conformant to C++03. Given this alias, it doesn't seem at all clear to me which of DW_LANG_C_plus_plus_98 and DW_LANG_C_plus_plus_03 would be the better choice, if both C++98 and C++03 must share a language tag... but I presume this has been discussed before. (It also doesn't seem clear whether it would be better to model "c++98" as an alias for "c++03".)

Yes, we don't have DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in clang for now. I guess this is because clang does not support DWARF 6. DWARF 6 is not officially released? Once DWARF 6 is released and clang starts to support DWARF 6, I think we should add the support for DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in the place that this patch changes.

New DWARF language codes are published ahead of the release of the next version of DWARF, so that they may be used by implementations without having to wait for new DWARF version.

It would therefore make sense to go ahead and add DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 now, without waiting, but only in the non-strict DWARF mode. (There would be the question of whether it would still make sense in the code to say "DwarfVersion >= 6" given that DWARF 6 would be otherwise unsupported... but I don't have a strong view on that question.)

I think it would be strange to check DwarfVersion >= 6 if we can not set dwarf version to 6 by anyways. Maybe we can first add the DWARF 6 support to the front end first(Need a wider discussion). This is the patch where -gdwarf-5 was introduced https://reviews.llvm.org/rG3cb592d6b60c. Seems it was also before DWARF 5 was official released.

Wed, Jun 16, 10:30 PM · debug-info, Restricted Project
dblaikie accepted D104272: [OpaquePtr] Mangle intrinsics with opaque pointers arguments.

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

So, it is not ambiguous. Does the patch look good to you?

Wed, Jun 16, 10:28 PM · Restricted Project

Tue, Jun 15

dblaikie added a comment to D104341: API to get Address Offset Section.

You can mark it abandoned by choosing that action just above the feedback text box

Tue, Jun 15, 6:55 PM · Restricted Project
dblaikie added a comment to D104272: [OpaquePtr] Mangle intrinsics with opaque pointers arguments.

I couldn't parse your comment, could you rephrase?

Tue, Jun 15, 5:52 PM · Restricted Project
dblaikie added a comment to D104342: [IR] convert warn-stack-size from module flag to fn attr.

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

Tue, Jun 15, 5:48 PM · Restricted Project, Restricted Project
dblaikie added a comment to D104341: API to get Address Offset Section.

Can you instead use the API more like the way llvm-symbolizer does - starting with the skeleton CU in the executable and it'll be able to load the dwp itself? (this codepath is already present and more tested, covers other sections like rnglists, etc, that you'll need too)

Hmm. I thought way below is it.

Only relevant code. DwCtx is context of binary.

DWPContext = DwCtx->getDWOContext(opts::DWPPath.c_str());
llvm::Optional<uint64_t> DWOId = DwarfUnit->getDWOId()
DWARFCompileUnit *DWOCU = DWPContext.get()->getDWOCompileUnitForHash(*DWOId);

Let me poke around llvm-symbolizer some more. Last time I looked I don't remember seeing low level APIs, and more high level ones.

Tue, Jun 15, 5:37 PM · Restricted Project
dblaikie added a comment to D104272: [OpaquePtr] Mangle intrinsics with opaque pointers arguments.

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

It seems what's after "{intrinsic}." is not important for IR parsing: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/IntrinsicInst.cpp#L127.

Tue, Jun 15, 4:53 PM · Restricted Project
dblaikie accepted D104340: [Demangle] Support Rust v0 mangling scheme in llvm::demangle.

Sounds good

Tue, Jun 15, 4:49 PM · Restricted Project
dblaikie added a comment to D104341: API to get Address Offset Section.

Can you instead use the API more like the way llvm-symbolizer does - starting with the skeleton CU in the executable and it'll be able to load the dwp itself? (this codepath is already present and more tested, covers other sections like rnglists, etc, that you'll need too)

Tue, Jun 15, 4:44 PM · Restricted Project
dblaikie added a comment to D104272: [OpaquePtr] Mangle intrinsics with opaque pointers arguments.

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Tue, Jun 15, 4:06 PM · Restricted Project
dblaikie added a comment to D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name..

There are several files other than CGBlocks.cpp in which FunctionDecl::Create is called to create fake FunctionDecls. Do those places have to be fixed too?

Tue, Jun 15, 2:30 PM · Restricted Project
dblaikie accepted D104082: [CodeGen] Don't create a fake FunctionDecl when generating block/block_byref copy/dispose helper functions.

C++ non-virtual thunks and global initialization functions (_GLOBAL__sub_I_*) have only linkage names.

Tue, Jun 15, 11:18 AM · Restricted Project
dblaikie added inline comments to D104271: llvm-dwarfdump: Print warnings on invalid DWARF.
Tue, Jun 15, 11:08 AM · Restricted Project

Mon, Jun 14

dblaikie added inline comments to D104151: [docs][OpaquePtr] Shuffle around the transition plan section.
Mon, Jun 14, 9:12 PM · Restricted Project
dblaikie added inline comments to D103928: [IR] make -warn-frame-size into a module attr.
Mon, Jun 14, 9:08 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D104271: llvm-dwarfdump: Print warnings on invalid DWARF.
Mon, Jun 14, 9:05 PM · Restricted Project
dblaikie added a comment to D104272: [OpaquePtr] Mangle intrinsics with opaque pointers arguments.

(generally looks like the right direction to me - sounds like @aeubanks has some good feedback/issues to address)

Mon, Jun 14, 8:59 PM · Restricted Project
dblaikie accepted D100670: [ADT] Add makeVisitor to STLExtras.h.

Looks good, thanks!

Mon, Jun 14, 7:00 PM · Restricted Project
dblaikie accepted D104119: [IR] Remove forward declaration of GraphTraits from Type.h.

Sounds OK - though might be worth considering #including "GraphTraits.h" rather than the forward declaration - makes maintenance a bit easier later on, if default template arguments were added to GraphTraits or other changes are made.

Mon, Jun 14, 6:52 PM · Restricted Project
dblaikie accepted D103719: [NFC][OpaquePtr] Avoid calling getPointerElementType().

Looks right to me

Mon, Jun 14, 6:48 PM · Restricted Project
dblaikie added a comment to D103966: [llvm] Add new DI Flag IsZeroSize for D101237 [[no_unique_address]].

DICompositeType has a "SizeInBits" field - can that be used to specify a size of 0, instead of adding a new flag to convey this?

Mon, Jun 14, 6:46 PM · Restricted Project

Sat, Jun 12

dblaikie committed rG02c718301b30: llvm-objcopy: fix section size truncation/extension when dumping sections (authored by dblaikie).
llvm-objcopy: fix section size truncation/extension when dumping sections
Sat, Jun 12, 7:30 PM
dblaikie added a comment to D103131: support debug info for alias variable.

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that.

Hi David,

with imported declaration patch and with current patch(in review or say gcc way) this case works ok(we can print type and value of newname)
$cat test.c
int oldname = 1;
extern int newname attribute((alias("oldname")));

but when we make newname static it works with current patch(in review or say gcc way) but it does not work with imported decl patch(https://reviews.llvm.org/D103131?id=347883).

Should we go with gcc way or am I missing something?
note: used gdb debugger.

An ideas what's happening when newname is static? Is the DWARF any different/interesting there? (since the DWARF for imported decl seems like it would have nothing to do with the linkange of the alias - since it doesn't refer to the actual alias in the object file, etc)

There not much different apart from extern has external attribute.
case 1) when newname is static

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000072, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000076)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 13.0.0 (git@github.com:llvm/llvm-project.git 4cd7169f5517167ef456e82c6dcae669bde6c725)")
              DW_AT_language    (DW_LANG_C99)
              DW_AT_name        ("test.c")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x0000000000000008)

0x0000002a:   DW_TAG_variable
                DW_AT_name      ("oldname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (1)
                DW_AT_location  (DW_OP_addr 0x0)

0x0000003f:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000046:   DW_TAG_variable
                DW_AT_name      ("newname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_declaration       (true)

0x00000051:   DW_TAG_imported_declaration
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_import    (0x00000046)
                DW_AT_name      ("newname")

0x0000005c:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000008)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (3)
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)

0x00000075:   NULL

case 2) when newname is extern

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000072, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000076)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 13.0.0 (git@github.com:llvm/llvm-project.git 4cd7169f5517167ef456e82c6dcae669bde6c725)")
              DW_AT_language    (DW_LANG_C99)
              DW_AT_name        ("test.c")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x0000000000000008)

0x0000002a:   DW_TAG_variable
                DW_AT_name      ("oldname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (1)
                DW_AT_location  (DW_OP_addr 0x0)

0x0000003f:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000046:   DW_TAG_variable
                DW_AT_name      ("newname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_declaration       (true)

0x00000051:   DW_TAG_imported_declaration
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_import    (0x00000046)
                DW_AT_name      ("newname")

0x0000005c:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000008)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (3)
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)

0x00000075:   NULL
Sat, Jun 12, 10:55 AM · debug-info, Restricted Project, Restricted Project

Fri, Jun 11

dblaikie added inline comments to D104151: [docs][OpaquePtr] Shuffle around the transition plan section.
Fri, Jun 11, 3:24 PM · Restricted Project
dblaikie accepted D104151: [docs][OpaquePtr] Shuffle around the transition plan section.

Sounds good to me - couple of optional comments/questions that could be clarified in the text.

Fri, Jun 11, 1:59 PM · Restricted Project
dblaikie accepted D103135: [OpaquePtr] Remove existing support for forward compatibility.

Fair enough

Fri, Jun 11, 1:10 PM · Restricted Project
dblaikie added a comment to D103888: [ADT] Remove APInt/APSInt toString() std::string variants.

Sounds OK.

I wouldn't mind the places that can use op<< to use that - not sure preserving the explicit radix argument is super high value. (I think people would generally assume that's the default)
Possible we could call it to_string, is std::to_string meant to be an ADL extension point, so that other types expose a to_string in their own associated namespace, etc?

We already have a llvm::to_string(V) wrapper inside ScopedPrinter.h that uses a temp llvm::raw_string_ostream to create the string - do you think that'd be good enough?

Fri, Jun 11, 12:00 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D103131: support debug info for alias variable.

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that.

Hi David,

with imported declaration patch and with current patch(in review or say gcc way) this case works ok(we can print type and value of newname)
$cat test.c
int oldname = 1;
extern int newname attribute((alias("oldname")));

but when we make newname static it works with current patch(in review or say gcc way) but it does not work with imported decl patch(https://reviews.llvm.org/D103131?id=347883).

Should we go with gcc way or am I missing something?
note: used gdb debugger.

Fri, Jun 11, 11:58 AM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D104082: [CodeGen] Don't create a fake FunctionDecl when generating block/block_byref copy/dispose helper functions.

Also, is it okay to emit the linkageName, but not the name for the helper functions? It seems okay to me, but I'm not sure.

Fri, Jun 11, 11:49 AM · Restricted Project

Thu, Jun 10

dblaikie accepted D103888: [ADT] Remove APInt/APSInt toString() std::string variants.

Sounds OK.

Thu, Jun 10, 12:50 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D103131: support debug info for alias variable.

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

Thu, Jun 10, 12:17 PM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D87302: [IRSim][IROutliner] Adding DebugInfo handling for IR outlined functions..

Does the outliner only run for commoning code, or can it run on a single function (for hot/cold reasons, or anything else)?

Thu, Jun 10, 12:08 PM · debug-info, Restricted Project
dblaikie updated subscribers of D103811: [docs] Set Phabricator as the tool for pre-commit code reviews.

So, I realize I probably should have mentioned this earlier on the llvm-dev thread, but is this really a change we should be making the week after Phabricator officially became an unsupported project:

https://admin.phacility.com/phame/post/view/11/phacility_is_winding_down_operations/

I feel like in practice we use Phabricator and not really pre-commit email reviews, so this might just be formalizing the practice, but Phabricator not being officially maintained means that we'll likely need to move off it at some point, so this policy change would likely be short lived.

Thu, Jun 10, 10:02 AM · Restricted Project

Tue, Jun 8

dblaikie committed rG8051a48e65cc: ORTRT: Add tests for string_view equality and inequality operators (authored by dblaikie).
ORTRT: Add tests for string_view equality and inequality operators
Tue, Jun 8, 5:54 PM
dblaikie committed rG4d9cc7c244e7: Add a couple of missing includes (authored by dblaikie).
Add a couple of missing includes
Tue, Jun 8, 5:54 PM
dblaikie committed rGcb09f2b10cbe: Rename compiler-rt/lib/orc/endian.h to endianness.h to avoid conflict with… (authored by dblaikie).
Rename compiler-rt/lib/orc/endian.h to endianness.h to avoid conflict with…
Tue, Jun 8, 5:54 PM
dblaikie added a comment to rG937c4cffd024: Fix implicit fall through compiler warning. NFCI..

This appears to change behavior - any chance of test coverage?

@dblaikie Not as far as I can work out - from what I can tell the integer types should have been legalized at that point, but preventing the implicit fall-through appeases compilers/analyzers and guarantees that we end up with a fatal error instead of a float dereference.

Tue, Jun 8, 8:51 AM
dblaikie committed rG49454ebc56ec: .clang-tidy: Disable misc-no-recursion in general/across the monorepo (authored by dblaikie).
.clang-tidy: Disable misc-no-recursion in general/across the monorepo
Tue, Jun 8, 8:40 AM
dblaikie committed rGc5d56fec502f: NFC: .clang-tidy: Inherit configs from parents to improve maintainability (authored by dblaikie).
NFC: .clang-tidy: Inherit configs from parents to improve maintainability
Tue, Jun 8, 8:27 AM
dblaikie closed D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability.
Tue, Jun 8, 8:26 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mon, Jun 7

dblaikie accepted D103848: [Demangle][Rust] Parse const backreferences.

Looks good!

Mon, Jun 7, 4:08 PM · Restricted Project
dblaikie accepted D103847: [Demangle][Rust] Parse type backreferences.

Looks good

Mon, Jun 7, 3:59 PM · Restricted Project
dblaikie requested review of D103843: Parallel-libs/.clang-tidy: Remove broken config and simplify existing one to use inheritance from the root .clang-tidy.
Mon, Jun 7, 2:25 PM · Restricted Project
dblaikie requested review of D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability.
Mon, Jun 7, 2:10 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
dblaikie accepted D103502: Bug 41152 - [DebugInfo] Better dumping of empty location expression .

Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.

Mon, Jun 7, 11:35 AM · debug-info, Restricted Project
dblaikie accepted D103811: [docs] Set Phabricator as the tool for pre-commit code reviews.

Sounds good

Mon, Jun 7, 9:03 AM · Restricted Project
dblaikie added a comment to D103811: [docs] Set Phabricator as the tool for pre-commit code reviews.

This looks about right to me - probably follow-up on the email threads to mention this has been sent for review/link to this review?

Mon, Jun 7, 8:38 AM · Restricted Project

Fri, Jun 4

dblaikie added a comment to D98477: [ADT] Add IntrusiveVariant, VariantTraits, and new STLForwardCompat.

Side thought: If this is going to use a prefix for the discriminator - could that be a base class that all types being used as union members have to derive from? No need for a macro, etc. Could use an offsetof comparison to make sure that not only is it a base class, but the first non-empty base class.

(still haven't been able to easily apply this locally - looks like it hit a conflict with c6f20d70a8c93ab3d50c9777c477fe040460a664 - though I thought arc would sync to the right version to apply the patch... not sure what's going on - might just wait to try that until the other patches have been committed)

Unfortunately having any non-static data members as part of a base class will make the alternative types non-standard-layout, which means we can't use the "common initial sequence" rule to pack them.

But would we need to use the common initial sequence rule? If the base object was at the start of the layout - a pointer to the start of the layout would be a valid pointer to the base object, yeah?

AFAIK that isn't true: we can't assume the base subobject is at the start of the layout of the derived object unless both are standard-layout.

My hope/thought was/is to use offsetof to check that the member is at the start of the object. Though, yes, for maximal portability, offsetof does require standard layout (though it seems both clang and gcc support it at least in some more cases than that: https://godbolt.org/z/Pjr6YEqMq )

In any case - even if it still leaves us with the standard layout restriction - using a base class rather than a macro+member seems like a nicer/more usable solution?

The moment we have non-static data members in both a derived class and its base class the derived class becomes non-standard-layout, though. Even just the following will not work:

struct VariantHeader {
  uint8_t Tag;
};

// SomeAlternative is not standard-layout
struct SomeAlternative : VariantHeader {
  int Foo;
};
Fri, Jun 4, 3:34 PM · Restricted Project
dblaikie accepted D103206: [ADT] Refactor enumerate unit tests.

Looks good, thanks!

Fri, Jun 4, 1:39 PM · Restricted Project
dblaikie added a comment to D102296: [ELF] getRelocatedSection: remove the check for ET_REL object file.

@dblaikie: mind accepting the diff then?

Fri, Jun 4, 1:39 PM · Restricted Project
dblaikie accepted D103223: [ADT][WIP] Proof of concept impl of generic visit for PointerUnion.
Fri, Jun 4, 1:21 PM · Restricted Project

Thu, Jun 3

dblaikie updated subscribers of D103667: [WIP] BPF: add support for DWARF_AT_LLVM_annotations attribute.

Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated?

Thu, Jun 3, 9:43 PM · Restricted Project, Restricted Project, debug-info
dblaikie added inline comments to D103502: Bug 41152 - [DebugInfo] Better dumping of empty location expression .
Thu, Jun 3, 9:22 PM · debug-info, Restricted Project