aprantl (Adrian Prantl)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 2 2013, 8:12 AM (228 w, 6 d)

Recent Activity

Yesterday

aprantl added inline comments to D35733: [DWARF] Added verification check for die ranges. If highPC is an address, then it should be greater or equal to lowPC for each range..
Fri, Jul 21, 3:42 PM
aprantl updated the diff for D35740: Fix PR33875 by distinguishing between DWO and clang modules.

No with testcase of sorts.

Fri, Jul 21, 3:41 PM
aprantl added inline comments to D35733: [DWARF] Added verification check for die ranges. If highPC is an address, then it should be greater or equal to lowPC for each range..
Fri, Jul 21, 3:41 PM
aprantl created D35740: Fix PR33875 by distinguishing between DWO and clang modules.
Fri, Jul 21, 3:33 PM
aprantl added inline comments to D35735: [ubsan] Null-check pointers in -fsanitize=vptr (PR33881).
Fri, Jul 21, 3:26 PM
aprantl accepted D35733: [DWARF] Added verification check for die ranges. If highPC is an address, then it should be greater or equal to lowPC for each range..

Seems reasonable, thanks!

Fri, Jul 21, 3:21 PM

Thu, Jul 20

aprantl added inline comments to D35715: Preserve typedef names in debug info for template type parameters.
Thu, Jul 20, 6:48 PM

Wed, Jul 19

aprantl added a comment to D35643: [DWARF] Added check that verifies that no abbreviation declaration has more than one attribute with the same name..

I think this looks fine now, but I'll let Fred do the final sign off.

Wed, Jul 19, 5:26 PM
aprantl added inline comments to D35643: [DWARF] Added check that verifies that no abbreviation declaration has more than one attribute with the same name..
Wed, Jul 19, 5:25 PM
aprantl added inline comments to D35643: [DWARF] Added check that verifies that no abbreviation declaration has more than one attribute with the same name..
Wed, Jul 19, 2:29 PM
aprantl accepted D35637: Fixing an issue with mixing functions from debug and non-debug CUs.

Thanks! Looks reasonable.

Wed, Jul 19, 11:54 AM

Tue, Jul 18

aprantl added inline comments to D35583: Debug Info: Add a file: field to DIImportedEntity.
Tue, Jul 18, 3:51 PM
aprantl accepted D34639: Fix DebugLoc propagation for unreachable LoadInst.

I think this looks good now.

Tue, Jul 18, 2:24 PM
aprantl updated the diff for D35583: Debug Info: Add a file: field to DIImportedEntity.

Forgot to git-add new clang test case input files.

Tue, Jul 18, 2:18 PM
aprantl added a comment to D35583: Debug Info: Add a file: field to DIImportedEntity.

Note that this review contains also the corresponding clang changes in the tools/ directory.

Tue, Jul 18, 2:15 PM
aprantl created D35583: Debug Info: Add a file: field to DIImportedEntity.
Tue, Jul 18, 2:15 PM

Mon, Jul 17

aprantl accepted D35444: [DWARF] Modification of code for the verification of .debug_info section..

LGTM with outstanding changes applied. Thanks!

Mon, Jul 17, 4:27 PM
aprantl added inline comments to D35444: [DWARF] Modification of code for the verification of .debug_info section..
Mon, Jul 17, 8:23 AM

Jun 20 2017

aprantl accepted D34418: [DWARF] Support for DW_FORM_strx3 and completed support for DW_FORM_strx{1,2,4}/consumer.

Seems generally straightforward/fine.

Jun 20 2017, 4:33 PM
aprantl added a comment to D34418: [DWARF] Support for DW_FORM_strx3 and completed support for DW_FORM_strx{1,2,4}/consumer.

This is just a general high-level comment: When adding new DWARF5 features it might also be a good idea to think about what checks could be added to DWARFVerifier to diagnose corrupt input (and potentially implement them as well :-)

Jun 20 2017, 4:24 PM

Jun 19 2017

aprantl added inline comments to D34359: [DWARF] Verification of the validity of the hash data offset and hash data DIEs in the the .apple_names section..
Jun 19 2017, 3:29 PM

Jun 14 2017

aprantl added a comment to D34214: Hide dbgs() stream for when built with -fmodules..

LGTM, thanks!

Jun 14 2017, 2:44 PM
aprantl accepted D34214: Hide dbgs() stream for when built with -fmodules..

That said, this patch is still obviously correct.

Jun 14 2017, 11:19 AM
aprantl added a comment to D34214: Hide dbgs() stream for when built with -fmodules..

I'm slightly confused why this works as a fix for the Debug build. Isn't this patch a no-op for a debug build?

Jun 14 2017, 11:18 AM
aprantl added inline comments to D34121: [ubsan] Teach the pointer overflow check that "p - <unsigned> <= p" (PR33430).
Jun 14 2017, 8:31 AM

Jun 13 2017

aprantl accepted D34177: [DWARF] Partial verification for .apple_names accelerator table in llvm-dwarfdump output..

This is good to go now. Thanks!

Jun 13 2017, 5:10 PM
aprantl added a comment to D34177: [DWARF] Partial verification for .apple_names accelerator table in llvm-dwarfdump output..

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

The DWARF v5 accelerator table is a direct evolution of the Apple accelerator table, so it is quite possible that we can share some of the implementation. Form a cursory glance it looks like at least the accessors for the hash table (bucket count, ...) are similar enough that they can share a common interface. We probably can't make that call until someone actually sits down and comes up with a design for how to implement the DWARF 5 accelerator tables, though. How about we revisit this question then? Hopefully this won't be too far out into the future.

Thanks for the info, if there's a reasonable chance to make it reasonably common then I'm happy to leave the naming as is for now.

Jun 13 2017, 4:36 PM
aprantl added a comment to D34177: [DWARF] Partial verification for .apple_names accelerator table in llvm-dwarfdump output..

Please subscribe 'llvm-commits' when you create a review, so the whole community can see the proposed patch.
(If you add llvm-commits later, we recommend that you paste the original description into a comment, because otherwise the mailing list never sees the original description.)

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

Jun 13 2017, 4:06 PM
aprantl added a reviewer for D34177: [DWARF] Partial verification for .apple_names accelerator table in llvm-dwarfdump output.: friss.
Jun 13 2017, 3:51 PM
aprantl added a comment to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].

Hm.. I think the other commit in the blamelist looks far more promising.

Jun 13 2017, 11:25 AM
aprantl added a comment to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].

It looks like this broke the stage2 build:

Jun 13 2017, 11:18 AM
aprantl accepted D33894: Align definition of DW_OP_plus with DWARF spec [3/3].

Looks good. Please watch the stage2 and LTO bots carefully after committing.

Jun 13 2017, 9:02 AM

Jun 12 2017

aprantl added a comment to D33894: Align definition of DW_OP_plus with DWARF spec [3/3].

IIRC git diff -U9999 should work just fine. In any case I am not going to review the binary file — I am just going to trust that it is what you say it is in the accompanying testcase :-)

Jun 12 2017, 9:39 AM
aprantl accepted D33894: Align definition of DW_OP_plus with DWARF spec [3/3].

This generally LGTM, there is only a bitcode upgrade test for DW_OP_minus missing. I'm marking the review as accepted under the condition that the missing test is added before committing.

Jun 12 2017, 9:19 AM
aprantl accepted D33892: Align definition of DW_OP_plus with DWARF spec [1/3].

Technically this is missing a DWARF backend test, but since the next review will rewrite all DW_OP_plus to DW_OP_plus_uconst anyway, this is fine.

Jun 12 2017, 9:09 AM

Jun 9 2017

aprantl added a comment to D33894: Align definition of DW_OP_plus with DWARF spec [3/3].

Ah ok, I see. The cleanest way to do this would be to

  1. Implement support for DW_OP_plus_constu in LLVM
  2. Switch over Clang to use DW_OP_plus_constu

3a. Retire the old DW_OP_plus and implement the bitcode upgrade in LLVM
3b. Retire the old DW_OP_minus (AFAIK not used by clang) and implement the bitcode upgrade in LLVM
(The last two can be one joined commit so we only need to bump the DIExpression version number once and keep the upgrade code simple).

Jun 9 2017, 11:10 AM
aprantl added a comment to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].

Thanks! I noticed that in the remainder of the patch there are still references to DW_OP_plus left, but they are completely untested. I would recommend removing all referenced to DW_OP_plus in this patch and then adding support for the "real" DW_OP_plus back in a separate commit, together with testcases.

Jun 9 2017, 8:42 AM

Jun 8 2017

aprantl added a comment to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].

Almost there! Couple more comments inline.

Jun 8 2017, 3:43 PM
aprantl added inline comments to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].
Jun 8 2017, 3:40 PM

Jun 7 2017

aprantl added inline comments to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].
Jun 7 2017, 11:20 AM

Jun 6 2017

aprantl accepted D33867: [DWARF] Introduce -brief command line option to llvm-dwarfdump.

Thanks, this LGTM now.

Jun 6 2017, 2:50 PM
aprantl added inline comments to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].
Jun 6 2017, 10:58 AM
aprantl added inline comments to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].
Jun 6 2017, 10:56 AM

Jun 5 2017

aprantl added a comment to D33867: [DWARF] Introduce -brief command line option to llvm-dwarfdump.

It looks like you accidentally created a diff between your last patch and the new version. When uploading a revised patch, please always upload the diff from svn trunk.

Jun 5 2017, 8:48 PM
aprantl requested changes to D33894: Align definition of DW_OP_plus with DWARF spec [3/3].

This also needs a bitcode upgrade to transform the old use of DW_OP_minus to the new one. See my comment on D33892 for details.

Jun 5 2017, 8:52 AM
aprantl accepted D33893: Align definition of DW_OP_plus with DWARF spec [2/3].

Thanks, this LGTM!

Jun 5 2017, 8:49 AM
aprantl requested changes to D33892: Align definition of DW_OP_plus with DWARF spec [1/3].

Thanks, this is great! The only thing that is missing here is a bitcode upgrade that transforms the old use of DW_OP_plus to DW_OP_plus_uconst. I recommend incrementing the "Version number" in operand 0 of DIExpression to distinguish between the old and new format. You can look at r300522 for an example of how to do this.

Jun 5 2017, 8:48 AM

Jun 3 2017

aprantl added inline comments to D33867: [DWARF] Introduce -brief command line option to llvm-dwarfdump.
Jun 3 2017, 10:40 AM
aprantl requested changes to D33867: [DWARF] Introduce -brief command line option to llvm-dwarfdump.

Thanks, I made a couple of comments inline.

Jun 3 2017, 10:39 AM

Jun 1 2017

aprantl added a comment to D33705: [CGVTables] Finalize SP before attempting to clone it.

Looks good.. Are you also planning to change DIBuilder to not finalize subprograms automatically any more (and not insert them into AllSubprograms)? (That will be the more impactful change as it will force all non-clang frontends to make a similar change).

Jun 1 2017, 1:28 PM
aprantl added a comment to D33749: [DWARF] Introduce Dump Options.

Thanks. I can commit this for you.

Jun 1 2017, 11:01 AM

May 31 2017

aprantl accepted D33704: [DIBuilder] Add a more fine-grained finalization method.

I'm fine with this change. I'm fine with either expecting the frontends to manually call finalize on each subprogram or with having DIBuilder do it. But let's have that discussion over in https://reviews.llvm.org/D33705.

May 31 2017, 4:21 PM
aprantl added a comment to D33704: [DIBuilder] Add a more fine-grained finalization method.

https://reviews.llvm.org/D33705 is the clang part of this. @dblaikie had some concerns (on the mailing list) - I had it only call it on cloned subprogram, but @dblaikie thought it might be more natural to call it immediately when irgen is done.

May 31 2017, 9:21 AM
aprantl added inline comments to D33704: [DIBuilder] Add a more fine-grained finalization method.
May 31 2017, 9:02 AM
aprantl added a comment to D33704: [DIBuilder] Add a more fine-grained finalization method.

I was a bit scared by the description at first, but this is safe. Will you only invoke it on functions to be cloned in clang or on all subprograms?

May 31 2017, 9:01 AM

May 30 2017

aprantl accepted D33655: [Cloning] Take another pass at properly cloning debug info.

Thanks Keno, this appears to be a far better approach. The only question that remains is whether / how to update the linkage name of the cloned DISubprogram. But I don't think that this needs to be solved in this patch.

May 30 2017, 8:17 AM

May 23 2017

aprantl accepted D32724: [Modules] Handle sanitizer feature mismatches when importing modules.

This is good from my point of view now. You might want to wait for an ok from @spyffe.

May 23 2017, 2:56 PM

May 19 2017

aprantl added a comment to D33370: Don't verify cross-imported bitcode in FunctionImporter.

It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier.

Oh right I forgot about this: lazy-loaded modules, even after materializing metadata, aren't passing the verified. This is super annoying.

After poking around a bit I came to the conclusion that it probably isn't even desirable to run the full-module verifier on each cross-imported module: Over the course of building the entire project the same module will be verified over and over again, which is a waste of resources. When we want to lazy-load metadata running a whole-module Verifier is also problematic.

There is something I'm missing: I thought the important part of the verifier was to be able to drop stalled debug information which is important for backward compatibility? This is the only reason I suggested doing it at this point of the process (before importing)...

May 19 2017, 3:45 PM
aprantl created D33370: Don't verify cross-imported bitcode in FunctionImporter.
May 19 2017, 3:17 PM
aprantl added a comment to D32368: LLVM C DIBuilder Creation APIs.

(Having unit tests will avoid

May 19 2017, 1:34 PM
aprantl added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

Thanks for the review!

May 19 2017, 10:40 AM
aprantl added a dependency for D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator.: D33360: Rewrite llvm-lto's codegen() using ThinCodeGenerator::run(). NFC-ish..
May 19 2017, 9:46 AM
aprantl added a dependent revision for D33360: Rewrite llvm-lto's codegen() using ThinCodeGenerator::run(). NFC-ish.: D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 19 2017, 9:46 AM
aprantl updated the diff for D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

Depending on https://reviews.llvm.org/D33360, get rid of the verifier invocation in codegen()

May 19 2017, 9:46 AM
aprantl created D33360: Rewrite llvm-lto's codegen() using ThinCodeGenerator::run(). NFC-ish..
May 19 2017, 9:38 AM

May 17 2017

aprantl added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

May 17 2017, 9:24 AM
aprantl accepted D33274: [DWARF] - Simplify RelocVisitor implementation..
May 17 2017, 9:01 AM

May 16 2017

aprantl added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

May 16 2017, 4:16 PM
aprantl added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

May 16 2017, 3:25 PM
aprantl added inline comments to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 16 2017, 3:18 PM
aprantl updated the diff for D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

Fix typos.

May 16 2017, 3:16 PM
aprantl updated the diff for D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

May 16 2017, 3:15 PM
aprantl added inline comments to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 16 2017, 1:49 PM
aprantl updated the diff for D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

What an entangled mess. This patch adds a boatload of testcases including the case where a function with broken debug info is cross-module-imported into a verified correct module. It also avoids re-verifying the module as much as possible.

May 16 2017, 1:47 PM
aprantl accepted D33194: [DWARF] - Cleanup relocations proccessing..
May 16 2017, 9:28 AM

May 15 2017

aprantl updated the diff for D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

Yes I think you are right. There are too many places that assume the Verifier is as function pass.
The updated patch disabled input verification in the pass manager builder and manually verifies the input.

May 15 2017, 4:07 PM
aprantl added a comment to D33194: [DWARF] - Cleanup relocations proccessing..

I'm assuming the checks are removed because they are already done elsewhere?

May 15 2017, 9:44 AM
aprantl added inline comments to D33184: [DWARF] - Make collectAddressRanges() return section index in addition to Low/High PC.
May 15 2017, 9:42 AM

May 12 2017

aprantl added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

This turns out to be quite ugly.
There are two different code paths, by their llvm-lto options:
-thinlto-action=run uses PassManagerBuilder and sets VerifyInput=true, which causes a VerifierLegacyPass to be scheduled
-thinlto-action=codegen (which mimics llc) doesn't.
The obvious correct fix for -thinlto-action=codegen is to also schedule a Verifier pass. That is easy — but wait.

May 12 2017, 8:20 PM
aprantl abandoned D31440: PR32382: Adapt to LLVM changes in DIExpression..
May 12 2017, 8:05 PM
aprantl added inline comments to D33155: [DWARFv5] Support FORM_strp in the line table header.
May 12 2017, 5:34 PM
aprantl added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

Interesting. The linker invocation I was debugging was using the

May 12 2017, 2:21 PM
aprantl added a comment to D31136: [DWARF] - Speedup handling of relocations in DWARFContextInMemory..

I don't have an informed opinion at the moment, besides "let's benchmark it", so please go ahead for now. Once we switch over LLDB to use the LLVM DWARF parser I will likely revisit this in more depth.

May 12 2017, 2:14 PM
aprantl added a comment to D33140: LTO: Don't verify modules twice in verifyMergedModuleOnce.

I don't think that is going to work, because of the report_fatal_error that also exits.

May 12 2017, 2:09 PM
aprantl created D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 12 2017, 1:47 PM
aprantl created D33140: LTO: Don't verify modules twice in verifyMergedModuleOnce.
May 12 2017, 10:29 AM

May 9 2017

aprantl added inline comments to D32724: [Modules] Handle sanitizer feature mismatches when importing modules.
May 9 2017, 4:51 PM
aprantl added inline comments to D29951: Load lazily the template specialization in multi-module setups..
May 9 2017, 11:22 AM

May 8 2017

aprantl accepted D32980: Use the frame index side table for byval and inalloca arguments.

This looks generally good (with outstanding comments addressed), thanks!

May 8 2017, 3:08 PM
aprantl added inline comments to D32975: Make it illegal for two Functions to point to the same DISubprogram.
May 8 2017, 1:45 PM
aprantl created D32975: Make it illegal for two Functions to point to the same DISubprogram.
May 8 2017, 12:57 PM
aprantl added a comment to D32920: Don't add DBG_VALUE instructions for static allocas in dbg.declare.

Interesting, right!

May 8 2017, 9:04 AM

May 5 2017

aprantl accepted D32920: Don't add DBG_VALUE instructions for static allocas in dbg.declare.

This does not really have anything to do with the code in this patch though. I just really want to understand it.

May 5 2017, 4:01 PM
aprantl added a comment to D32920: Don't add DBG_VALUE instructions for static allocas in dbg.declare.

I totally understand that it doesn't need to be tracked in the MMI table and in a DEBUG_VALUE, my question was that it sounded to my like you implied that the DWARF backend actively ignores DEBUG_VALUEs pointing to static allocas which I found really surprising (as there are legitimate reasons for allocas to be described by a DEBUG_VALUE) .

May 5 2017, 3:55 PM
aprantl added a reviewer for D32724: [Modules] Handle sanitizer feature mismatches when importing modules: dexonsmith.
May 5 2017, 3:45 PM
aprantl added a comment to D32724: [Modules] Handle sanitizer feature mismatches when importing modules.

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

May 5 2017, 3:45 PM
aprantl added a comment to D32920: Don't add DBG_VALUE instructions for static allocas in dbg.declare.

An llvm.dbg.declare of a static alloca is always added to the
MachineFunction dbg variable map, so these values are entirely
redundant.

May 5 2017, 2:33 PM
aprantl resigned from D32821: Add DWARF verifiers to verify address ranges are correct and scoped correctly..

David, are all your concerns addressed?
Otherwise from my end this change LGTM (with all outstanding issues addressed).

May 5 2017, 9:31 AM
aprantl added inline comments to D32821: Add DWARF verifiers to verify address ranges are correct and scoped correctly..
May 5 2017, 9:24 AM
aprantl added inline comments to D32865: [llvm-dwarfdump] - Print an error message if section decompression failed..
May 5 2017, 8:29 AM